Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8644 )
Change subject: KUDU-1097 (patch 5a): Implement a bulk config change API ...................................................................... Patch Set 6: (6 comments) http://gerrit.cloudera.org:8080/#/c/8644/3/src/kudu/consensus/consensus.proto File src/kudu/consensus/consensus.proto: http://gerrit.cloudera.org:8080/#/c/8644/3/src/kudu/consensus/consensus.proto@168 PS3, Line 168: required bytes tablet_id = 2 > Mind setting this as the first field of the message? That way, I think, it They are already in the order consistent with the other messages in this file. Or are you talking about wire order? If so why does that matter? http://gerrit.cloudera.org:8080/#/c/8644/3/src/kudu/integration-tests/raft_config_change-itest.cc File src/kudu/integration-tests/raft_config_change-itest.cc: http://gerrit.cloudera.org:8080/#/c/8644/3/src/kudu/integration-tests/raft_config_change-itest.cc@359 PS3, Line 359: > nit: does it make sense to check for the exact error type here? will do http://gerrit.cloudera.org:8080/#/c/8644/3/src/kudu/integration-tests/raft_config_change-itest.cc@375 PS3, Line 375: /*replace=*/ false, /*promote=*/ false } }); > mind adding a case to test adding/removing multiple voter replicas? Good call, will do. http://gerrit.cloudera.org:8080/#/c/8644/6/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc File src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc: http://gerrit.cloudera.org:8080/#/c/8644/6/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@a650 PS6, Line 650: : : : : : : : > Ah, that's probably because now change which does not change anything is ju Do you think we should try to detect a no-op and reject it? I'm not sure it's very important to reject MODIFY_PEER no-ops, and the obvious way to detect it seems prone to human error in the future, so I just removed that behavior. http://gerrit.cloudera.org:8080/#/c/8644/6/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@a665 PS6, Line 665: : : : : : > ditto same http://gerrit.cloudera.org:8080/#/c/8644/3/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8644/3/src/kudu/tserver/tablet_service.cc@1044 PS3, Line 1044: ChangeConfigResponsePB > nit: just to clarify: we don't anticipate this to change in the context of I don't think so. -- To view, visit http://gerrit.cloudera.org:8080/8644 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I928a1622d48049c9ad3223b76549a2822bccc30c Gerrit-Change-Number: 8644 Gerrit-PatchSet: 6 Gerrit-Owner: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Mon, 27 Nov 2017 23:42:50 +0000 Gerrit-HasComments: Yes
