Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11251 )
Change subject: KUDU-2245 Graceful leadership transfer ...................................................................... Patch Set 7: (14 comments) Few more nits. http://gerrit.cloudera.org:8080/#/c/11251/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11251/7//COMMIT_MSG@10 PS7, Line 10: original Raft thesis I think a reference (like https://raft.github.io/raft.pdf) would not hurt. http://gerrit.cloudera.org:8080/#/c/11251/7//COMMIT_MSG@16 PS7, Line 16: beings begins Maybe replace with 'starts' ? http://gerrit.cloudera.org:8080/#/c/11251/7//COMMIT_MSG@33 PS7, Line 33: Leadership transfer should usually be much faster and it : allows the client to select the new leader among current voters. Did you do any measurements in that regard? If yes, I think it would be nice to mention the summary of your findings in this commit message. http://gerrit.cloudera.org:8080/#/c/11251/7//COMMIT_MSG@42 PS7, Line 42: WIP Is it still WIP patch? The WIP tag in the first line is gone. Should I assume those 3 items below are still TODO or already done? http://gerrit.cloudera.org:8080/#/c/11251/7/src/kudu/consensus/consensus-test-util.h File src/kudu/consensus/consensus-test-util.h: http://gerrit.cloudera.org:8080/#/c/11251/7/src/kudu/consensus/consensus-test-util.h@299 PS7, Line 299: virtual nit: drop 'virtual' since 'override' is used. http://gerrit.cloudera.org:8080/#/c/11251/7/src/kudu/consensus/consensus-test-util.h@355 PS7, Line 355: virtual nit: drop http://gerrit.cloudera.org:8080/#/c/11251/7/src/kudu/consensus/consensus-test-util.h@482 PS7, Line 482: virtual nit: drop http://gerrit.cloudera.org:8080/#/c/11251/7/src/kudu/consensus/consensus_queue.h File src/kudu/consensus/consensus_queue.h: http://gerrit.cloudera.org:8080/#/c/11251/7/src/kudu/consensus/consensus_queue.h@362 PS7, Line 362: boost:none boost::none http://gerrit.cloudera.org:8080/#/c/11251/7/src/kudu/consensus/consensus_queue.h@365 PS7, Line 365: WatchForSuccessor nit: maybe, rename to BeginWatchForSuccessor() to pair with the EndWatchForSuccessor() below. http://gerrit.cloudera.org:8080/#/c/11251/7/src/kudu/consensus/consensus_queue.h@559 PS7, Line 559: designated_successor_ designated_successor_uuid_ ? http://gerrit.cloudera.org:8080/#/c/11251/7/src/kudu/consensus/consensus_queue.h@593 PS7, Line 593: virtual void NotifyPeerToStartElection(const std::string& peer_uuid) = 0; Add documentation. http://gerrit.cloudera.org:8080/#/c/11251/7/src/kudu/integration-tests/raft_consensus-itest.cc File src/kudu/integration-tests/raft_consensus-itest.cc: http://gerrit.cloudera.org:8080/#/c/11251/7/src/kudu/integration-tests/raft_consensus-itest.cc@3001 PS7, Line 3001: TEST_F(RaftConsensusITest, TestLeaderTransferWhenFollowerFallsBehindLeaderGC) { Nice test. How long does it run? If more than 5 seconds, consider adding if (!AllowSlowTests()) { LOG(WARNING) << "test is skipped; set KUDU_ALLOW_SLOW_TESTS=1 to run"; return; } http://gerrit.cloudera.org:8080/#/c/11251/7/src/kudu/integration-tests/raft_consensus-itest.cc@3054 PS7, Line 3054: : nit: two extra lines http://gerrit.cloudera.org:8080/#/c/11251/7/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/11251/7/src/kudu/tools/kudu-admin-test.cc@1514 PS7, Line 1514: , { } nit: drop -- 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: 7 Gerrit-Owner: Will Berkeley <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Fengling Wang <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Wed, 12 Sep 2018 06:42:29 +0000 Gerrit-HasComments: Yes
