Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8245 )
Change subject: KUDU-2048. consensus: only evict unresponsive nodes if remaining voters are viable ...................................................................... Patch Set 4: (8 comments) Nice cleanup. Looks good. http://gerrit.cloudera.org:8080/#/c/8245/4/src/kudu/consensus/consensus_queue.h File src/kudu/consensus/consensus_queue.h: http://gerrit.cloudera.org:8080/#/c/8245/4/src/kudu/consensus/consensus_queue.h@270 PS4, Line 270: doc still needs doc http://gerrit.cloudera.org:8080/#/c/8245/4/src/kudu/consensus/consensus_queue.cc File src/kudu/consensus/consensus_queue.cc: http://gerrit.cloudera.org:8080/#/c/8245/4/src/kudu/consensus/consensus_queue.cc@97 PS4, Line 97: PeerStatusToString Since we expose PeerStatus to other classes in the ConsensusQueue API, this print helper should probably also be declared in the .h file. Or if not publicly declared, it should be static. http://gerrit.cloudera.org:8080/#/c/8245/4/src/kudu/consensus/consensus_queue.cc@394 PS4, Line 394: &= Clever, but I think this would be more readable written as: if (peer->last_exchange_status != PeerStatus::OK) viable = false; as well as below http://gerrit.cloudera.org:8080/#/c/8245/4/src/kudu/consensus/consensus_queue.cc@410 PS4, Line 410: WARNING Maybe this should be INFO since it's not really a warning. Do you see a lot of people running Kudu at WARNING log level? http://gerrit.cloudera.org:8080/#/c/8245/4/src/kudu/consensus/consensus_queue.cc@699 PS4, Line 699: th the http://gerrit.cloudera.org:8080/#/c/8245/4/src/kudu/consensus/consensus_queue.cc@713 PS4, Line 713: serialize deserialize? http://gerrit.cloudera.org:8080/#/c/8245/4/src/kudu/consensus/consensus_queue.cc@834 PS4, Line 834: // That's currently how we can detect that we able to connect to a peer. Would you mind deleting this comment while you're in here? I don't think it adds value anymore and the grammar kind of grates. http://gerrit.cloudera.org:8080/#/c/8245/4/src/kudu/integration-tests/tablet_replacement-itest.cc File src/kudu/integration-tests/tablet_replacement-itest.cc: http://gerrit.cloudera.org:8080/#/c/8245/4/src/kudu/integration-tests/tablet_replacement-itest.cc@335 PS4, Line 335: "--follower_unavailable_considered_failed_sec=5" }; I think we will get a shorter test time with --consensus_rpc_timeout_ms=3 or something similar here. -- To view, visit http://gerrit.cloudera.org:8080/8245 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I673f5b8a58b3954ea28066ecb334b3fdd60e7adb Gerrit-Change-Number: 8245 Gerrit-PatchSet: 4 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Fri, 03 Nov 2017 00:30:53 +0000 Gerrit-HasComments: Yes
