Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11251 )
Change subject: KUDU-2245 Graceful leadership transfer ...................................................................... Patch Set 17: (5 comments) Finished going through the tests, this all looks good but I wonder if we are missing coverage in the graceful, non-designated case. http://gerrit.cloudera.org:8080/#/c/11251/17/src/kudu/consensus/consensus_queue.h File src/kudu/consensus/consensus_queue.h: http://gerrit.cloudera.org:8080/#/c/11251/17/src/kudu/consensus/consensus_queue.h@468 PS17, Line 468: // Determine if 'peer' is fully caught up to the leader. If it is, notify : // observers. : void DetermineIfCaughtUp(const TrackedPeer& peer, const ConsensusStatusPB& status); Let's rename this method to something like TransferLeadershipIfNeeded() and have the doc comment explain its full responsibilities, such as: // If there is a graceful leadership change underway, notify queue observers // to initiate leadership transfer to the specified peer under the following // conditions: // * 'peer' has fully caught up to the leader // * 'peer' is the designated successor, or no successor was designated void TransferLeadershipIfNeeded(const TrackedPeer& peer, const ConsensusStatusPB& status); http://gerrit.cloudera.org:8080/#/c/11251/17/src/kudu/integration-tests/raft_consensus-itest.cc File src/kudu/integration-tests/raft_consensus-itest.cc: http://gerrit.cloudera.org:8080/#/c/11251/17/src/kudu/integration-tests/raft_consensus-itest.cc@3024 PS17, Line 3024: TEST_F(RaftConsensusITest, TestLeaderTransferWhenFollowerFallsBehindLeaderGC) { please add a test comment with something like: // Designating graceful leadership transfer to a follower that cannot catch up should eventually fail. http://gerrit.cloudera.org:8080/#/c/11251/17/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/11251/17/src/kudu/tools/kudu-admin-test.cc@1315 PS17, Line 1315: TEST_F(AdminCliTest, TestLeaderTransferToEvictedPeer) { please add a test comment: // Leader should reject requests to transfer leadership to a non-member of the config. http://gerrit.cloudera.org:8080/#/c/11251/17/src/kudu/tools/kudu-admin-test.cc@1377 PS17, Line 1377: TEST_F(AdminCliTest, TestLeaderTransferToNonVoter) { please add a test comment: // Leader should reject requests to transfer leadership to a non-voter of the config. http://gerrit.cloudera.org:8080/#/c/11251/17/src/kudu/tools/kudu-admin-test.cc@1571 PS17, Line 1571: "leader_step_down", This seems to be the only test we have for non-designated graceful leader step down. Can we add an assertion to check that the leader actually changed, in the graceful case, or add a test for non-designated graceful stepdown? -- To view, visit http://gerrit.cloudera.org:8080/11251 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic97343af9eb349556424c999799ed5e2941f0083 Gerrit-Change-Number: 11251 Gerrit-PatchSet: 17 Gerrit-Owner: Will Berkeley <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Fengling Wang <[email protected]> Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Wed, 17 Oct 2018 19:45:25 +0000 Gerrit-HasComments: Yes
