David Ribeiro Alves has posted comments on this change. Change subject: KUDU-871. Support tombstoned voting ......................................................................
Patch Set 6: (7 comments) leaving for luch, more comments forthcoming http://gerrit.cloudera.org:8080/#/c/6960/6/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: PS6, Line 1519: LOG_WITH_PREFIX_UNLOCKED(INFO) << "voting while tombstoned based on last-logged opid " : << local_last_logged_opid; move this to the else block above and thus avoid the extra if? PS6, Line 2538: const optional<OpId>& tombstone_last_logged_opid whats the point of passing an opid here if you're only checking for its presence? also you mention on LOC 1508 "in which case we are guaranteed that // 'tombstone_last_logged_opid' is set by CheckSafeToVoteUnlocked()" but that is not happening or am I missing something? http://gerrit.cloudera.org:8080/#/c/6960/6/src/kudu/consensus/raft_consensus.h File src/kudu/consensus/raft_consensus.h: PS6, Line 124: static Status Create(ConsensusOptions options, : RaftPeerPB local_peer_pb, : scoped_refptr<ConsensusMetadataManager> cmeta_manager, : ThreadPool* raft_pool, : std::shared_ptr<RaftConsensus>* consensus_out); didn't you delete this a while back? PS6, Line 271: term nit: current_term() would be more explicit PS6, Line 292: Tombstoned voting not super clear what "tombstoned voting" is maybe explain it somewhere? PS6, Line 354: RaftConsensus has been stopped. maybe explain what this means in the context of the other states? something like: "RaftConsensus has been stopped. No more transactions or commits are accepted but will still reply to election requests if tombstoned." http://gerrit.cloudera.org:8080/#/c/6960/6/src/kudu/integration-tests/tombstoned_voting-itest.cc File src/kudu/integration-tests/tombstoned_voting-itest.cc: PS6, Line 195: // Ask TS1 for a vote that should be denied (old last-logged opid). : s = itest::RequestVote(ts1_ets, tablet_id, "B", current_term, old_opid, : /*ignore_live_leader=*/ true, /*is_pre_election=*/ false, kTimeout); can you add a test for same candidate same term but with an opid that is greater that the tombstoned replica's op_id? -- To view, visit http://gerrit.cloudera.org:8080/6960 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia19d75b185299443b27f41e468bbae20065e7570 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> 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-HasComments: Yes
