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 3: (14 comments) looks good overall, some nits 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: virtual void NotifyPeerToPromote(const std::string& peer_uuid, nit: add comment/doc? http://gerrit.cloudera.org:8080/#/c/8633/2/src/kudu/consensus/consensus_queue.cc File src/kudu/consensus/consensus_queue.cc: http://gerrit.cloudera.org:8080/#/c/8633/2/src/kudu/consensus/consensus_queue.cc@909 PS2, Line 909: Factor this out into a dedicated function indeed 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: // TODO(mpercy): FIXME Could you be more specific on what's is wrong here? 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: Notified about a promotion requested for $0: " nit: may be just "attempt to promote $0" 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: Substitute("$0: current term ($1) is different from the term of request ($2), aborting", msg, term, current_term) 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: Substitute("$0: config opid index mismatch: requested at $1, current committed $2, aborting", msg, committed_config_opid_index, current_committed_config_opid_index); 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: Substitute("$0: cannot promote non-voter when another config change operation in progress, aborting"); 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. But so far the semantics of non-clobbering not-specified attributes looks good to me. 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: consensus::RaftPeerAttrsPB() nit: would {} be sufficient here instead of consensus::RaftPeerAttrsPB() ? 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: 3 nit: I think it would be more expressive to set corresponding gflag prior to that: constexpr int kNumReplicas = 3; FLAGS_num_replicas = kNumReplicas; ASSERT_OK(inspect_->WaitForReplicaCount(kNumReplicas)); http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/integration-tests/raft_config_change-itest.cc@194 PS3, Line 194: LOG(INFO) << "Looking for tablet leader..."; As for this and other LOG(INFO) -- is it crucial to have those? http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/integration-tests/raft_config_change-itest.cc@211 PS3, Line 211: } nit: consider adding NO_FATALS(cluster_->AssertNoCrashes()); 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: RaftPeerAttrsPB() nit: would {} be sufficient here instead? http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/integration-tests/raft_consensus-itest.cc@1544 PS3, Line 1544: RaftPeerAttrsPB() ditto -- 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: 3 Gerrit-Owner: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Thu, 23 Nov 2017 03:08:43 +0000 Gerrit-HasComments: Yes
