Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8633 )
Change subject: KUDU-1097 (patch 3): Implement promotion of NON_VOTER replicas ...................................................................... Patch Set 4: (5 comments) http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/consensus/raft_consensus.cc@833 PS3, Line 833: msg << "notified when " : << "committed config had opid index " : << committed_config_opid_index << ", but now " : << "the committed config has opid index " : << current_committed_config_opid_index : << ". Doing nothing." > Again, I think the more verbose message I wrote will be easier for users to It's fine with me. I was just wanted to express the same amount of information in less words. Also, code-wise, I would expect using Substitute() to be more readable than multi-line string with 'operator <<' http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/consensus/raft_consensus.cc@843 PS3, Line 843: msg << "there is already a config change operation " : << "in progress. Unable to promote follower until it " : << "completes. Doing nothing."; > I don't really see how what you wrote is an improvement over what is alread It's more concise and readable: it delivers the same amount of actionable information, and it's less pollution in the log. But if you prefer this longer message, it's fine with me. http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/consensus/raft_consensus.cc@1741 PS3, Line 1741: handle a non-voter that had both its : // "promote" and "replace" flags set. > Do you think we should disallow it? I'm saying the particular case of a non-voter with both 'replace' and 'promote' attributes set is not applicable in our current design, if I'm not mistaken. So, maybe this particular part of the comment should be removed. In general case, I think we should allow handling multiple attributes in the ChangeConfig request. http://gerrit.cloudera.org:8080/#/c/8633/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/8633/3/src/kudu/integration-tests/raft_config_change-itest.cc@170 PS3, Line 170: c > Hmm, there are a couple things here: Ah, sorry -- I thought RaftConfigChangeITest was a subclass of TabletServerIntegrationTestBase. The point with that update was to make sure we requested 3 replicas and we got 3 replicas running, not something like requested 5 replicas and got just 3 running. If it's clear there are 3 replicas only, then it's fine as is then. http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/integration-tests/raft_config_change-itest.cc@194 PS3, Line 194: SERT_EVENTUALLY([&] { > I just left this in for debugging in case this test ever fails, but I can r sgtm: if it helps with debugging, it makes sense to leave those then. -- To view, visit http://gerrit.cloudera.org:8080/8633 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ife6f59658cb2f38d087d57c244ad811010a91fef Gerrit-Change-Number: 8633 Gerrit-PatchSet: 4 Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Mon, 27 Nov 2017 07:26:04 +0000 Gerrit-HasComments: Yes
