Mike Percy 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: (13 comments) http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/consensus/consensus_queue.h File src/kudu/consensus/consensus_queue.h: http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/consensus/consensus_queue.h@553 PS3, Line 553: // Notify the observer that the specified peer is ready to be promoted from > nit: add comment/doc? Done http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/consensus/consensus_queue.cc File src/kudu/consensus/consensus_queue.cc: http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/consensus/consensus_queue.cc@1308 PS3, Line 1308: observer->NotifyPeerTo > Could you be more specific on what's is wrong here? oops, meant to remove this 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@817 PS3, Line 817: attempt to promote peer $0: ", peer_uuid); > nit: may be just Done http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/consensus/raft_consensus.cc@823 PS3, Line 823: msg << "requested in " : << "previous term " << term : << ", but a leader election " : << "likely occurred since then. Doing nothing."; > maybe just: I think this is friendlier to users reading the log messages. We print the current term automatically in the RaftConsensus LogPrefix. 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." > maybe just: Again, I think the more verbose message I wrote will be easier for users to understand when looking through the log files. 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."; > maybe just: I don't really see how what you wrote is an improvement over what is already there. 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. > I'm not sure this is ever needed at all. Do you think we should disallow it? http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/integration-tests/cluster_itest_util.h File src/kudu/integration-tests/cluster_itest_util.h: http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/integration-tests/cluster_itest_util.h@279 PS3, Line 279: {}, > nit: would {} be sufficient here instead of consensus::RaftPeerAttrsPB() ? Done 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 > nit: Hmm, there are a couple things here: 1) Those gflags don't do anything in the external mini cluster, that is a special thing that only works in raft_consensus-itest, and I think it's overly magical and kind of bad to set global variables like that 2) Isn't it clear from the context that we are simply waiting for 3 replicas? The TestWorkload defaults to 3 replicas, because Kudu defaults to creating 3 replicas, so that's why it doesn't appear more than once in this function. If it had, I would have used a constant for it. That said, if you think provides additional clarity to declare constexpr int kNumReplicas = 3 to use here only, I can add it. http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/integration-tests/raft_config_change-itest.cc@194 PS3, Line 194: SERT_EVENTUALLY([&] { > As for this and other LOG(INFO) -- is it crucial to have those? I just left this in for debugging in case this test ever fails, but I can remove it. http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/integration-tests/raft_config_change-itest.cc@211 PS3, Line 211: NO_FATALS(cluster_->AssertNoCrashes()); > nit: consider adding Done http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/integration-tests/raft_consensus-itest.cc File src/kudu/integration-tests/raft_consensus-itest.cc: http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/integration-tests/raft_consensus-itest.cc@1536 PS3, Line 1536: {}, -1, &error_co > nit: would {} be sufficient here instead? Done http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/integration-tests/raft_consensus-itest.cc@1544 PS3, Line 1544: {}, committed_opi > ditto Done -- 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 <mpe...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Mon, 27 Nov 2017 06:58:03 +0000 Gerrit-HasComments: Yes
