Todd Lipcon 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) 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 Done 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 Done 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: I started to change this but then I found it a bit harder to read, since the comments are framed in positive terms (a peer is viable if A _and_ B _and_ C) rather than negative terms (a peer is not viable if A _or_ B _or_ C). Negating all the comments seemed a little harder to follow - it's sort of a double-negative. 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 oh this was just a debug log i accidentally left in. I usually use WARNING or ERROR when I'm actively debugging something so that the log messages get colored and pop out easily on my screen :) http://gerrit.cloudera.org:8080/#/c/8245/4/src/kudu/consensus/consensus_queue.cc@699 PS4, Line 699: th > the Done http://gerrit.cloudera.org:8080/#/c/8245/4/src/kudu/consensus/consensus_queue.cc@713 PS4, Line 713: serialize > deserialize? Done 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 Done 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 o copied from all the tests above. I think given how flaky these sorts of tests have been lately I'd rather be conservative here. Too short of an eviction timeout and maybe it wouldn't do what we expected? -- 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 <t...@apache.org> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Wed, 15 Nov 2017 20:35:06 +0000 Gerrit-HasComments: Yes
