Todd Lipcon has posted comments on this change.

Change subject: KUDU-871. Support tombstoned voting

Patch Set 4:


did a first pass, probably will have more comments after a second pass but 
figured I'd send these along
File src/kudu/consensus/

Line 188: Status RaftConsensus::Init() {
I think the DCHECK on state here was useful, even if it needs to be broadened a 
bit now

PS4, Line 1493: tombstone_last_logged_opid ? *tombstone_last_logged_opid :
could we simplify by always using 'max()' of the entry from the metadata and 
the entry from our log (if we have a log)

Line 1497:     LOG_WITH_PREFIX_UNLOCKED(INFO) << "voting while tombstoned";
any way to improve this with a bit more info?

Line 2004:       state_ = new_state;
given this is in every case, move it down below the switch?

PS4, Line 2515: (!tombstone_last_logged_opid
not 100% following why this portion of the condition is necessary

PS4, Line 2520: state_ == kShutdown
should we instead be checking the expected state (ie kStopped)? aren't there 
some other states we might be in where we don't want to vote?

Line 2634:   return kMinimumTerm;
under what circumstances do we hit this case? It seems like cmeta_ should be 
set in Init() and we shouldn't ever be calling functions on RaftConsensus until 
after a successful Init() (see other comment about changing to a factory 
function so it's more clear that we never expose a 
constructed-by-not-initialized instance)

Line 2685:   if (cmeta_) {
File src/kudu/integration-tests/cluster_itest_util.h:

PS4, Line 239:                    boost::optional<bool> ignore_live_leader,
             :                    boost::optional<bool> is_pre_election,
what's the point of making these optional? don't they have well-defined 
defaults (false) anyway?
File src/kudu/tablet/metadata.proto:

Line 135: // Tablet states are sent in TabletReports and kept in TabletReplica.
what effect will these changes have if there is a rolling upgrade with an old 
master and new tablet servers? do we need to force an incompatibility so that 
people upgrade masters first? or is it safe to have these show up as 'UNKNOWN' 
on an old-version master?
File src/kudu/tablet/

Line 133:   set_state(INITIALIZED);
maybe moot based on the suggestion of a factory method, but if a method like 
this remains, the state change should wait until the method is successful

Line 272:     // Release mem tracker resources.
is this comment stale?

PS4, Line 407:   LOG_WITH_PREFIX(INFO) << "Fetched status for addr " << &state_
             :                         << ": " << 
leftover debug print?

Line 427:   CHECK_EQ(INITIALIZED, state_);
redundant with the DCHECK in set_state
File src/kudu/tablet/tablet_replica.h:

PS4, Line 75:   TabletReplica(scoped_refptr<TabletMetadata> meta,
scoped_refptr<consensus::ConsensusMetadataManager> cmeta_manager,
            :                 consensus::RaftPeerPB local_peer_pb,
            :                 ThreadPool* apply_pool,
            :                 Callback<void(const std::string& reason)> 
            :   // Initializes RaftConsensus.
            :   Status Init(ThreadPool* raft_pool);
instead of having this split "construct and then init" how about a factory 
method which would have a similar signature to the original constructor?


static Status Create(scoped_refptr<TabletMetadata> meta,
                     scoped_refptr<TabletReplica>* replica);

which does the construction and Init()?

That way there is less externally-visible lifecycle for callers to think about 
(and maybe could reduce one of the enum values too)?

PS4, Line 96: tombstoned 
voting in general, right? ie what would happen if you called Stop() but the 
data wasn't tombstoned, and you asked to vote?

PS4, Line 169:   // If any peers in the consensus configuration lack permanent 
uuids, get them via an
             :   // RPC call and update.
             :   // TODO: move this to raft_consensus.h.
             :   Status UpdatePermanentUuids();
can you remove this while you're here? seems to be dead

Line 277:   // Wait until the TabletReplica is fully in STOPPED state.
File src/kudu/tserver/

Line 947:   tablet::TabletDataState data_state = 
aren't all these state checks racy? after you grab this here, there's nothing 
preventing the tablet from entering DELETED or COPYING state? It seems like at 
this layer it should just be conditioned on whether we can find a Consensus 
object, and then the actual RaftConsensus::RequestVote() call should be 
responsible for ensuring it's in an appropriate state?

PS4, Line 966:     if (OpIdBiggerThan(tmp_last_logged_opid, MinimumOpId())) {
when would this equal MinimumOpId? if it's never been tombstoned?

Would it be safe to just pass this in always as a non-optional, and then in 
RequestVote always just take max(log_->GetLastLoggedOpId(), 
File src/kudu/tserver/

Line 460:   if (!consensus || !consensus->IsRunning()) {
instead of this, can you make Consensus::DumpStatusHtml dump something 
reasonable in the case that it is STOPPED?

Otherwise it seems like this is a racy check since you have nothing ensuring 
that the state doesn't change between here and L464

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia19d75b185299443b27f41e468bbae20065e7570
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <>
Gerrit-Reviewer: Alexey Serbin <>
Gerrit-Reviewer: David Ribeiro Alves <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to