David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-871. Support tombstoned voting

Patch Set 6:


leaving for luch, more comments forthcoming

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 
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?

File src/kudu/consensus/raft_consensus.h:

PS6, Line 124: static Status Create(ConsensusOptions options,
             :                        RaftPeerPB local_peer_pb,
             :                        scoped_refptr<ConsensusMetadataManager> 
             :                        ThreadPool* raft_pool,
             :                        std::shared_ptr<RaftConsensus>* 
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."

File src/kudu/integration-tests/tombstoned_voting-itest.cc:

PS6, Line 195:   // Ask TS1 for a vote that should be denied (old last-logged 
             :   s = itest::RequestVote(ts1_ets, tablet_id, "B", current_term, 
             :                          /*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

Reply via email to