Mike Percy has posted comments on this change.

Change subject: KUDU-871. Support tombstoned voting

Patch Set 4:


File src/kudu/consensus/raft_consensus.cc:

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

PS4, Line 1493: tombstone_last_logged_opid ? *tombstone_last_logged_opid :
> could we simplify by always using 'max()' of the entry from the metadata an
I don't think so; see my response to the same question in tablet_service.cc

However, I do think we should use the latest opid in the log if Consensus is 
currently running, since it is guaranteed to be correct in that case.

Line 1497:     LOG_WITH_PREFIX_UNLOCKED(INFO) << "voting while tombstoned";
> any way to improve this with a bit more info?
I'll add the tombstone_last_logged_opid here but I can't think of a reason to 
log more than that because we already log the details whenever we vote.

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

Line 2513: Status RaftConsensus::CheckSafeToVoteUnlocked(optional<OpId> 
tombstone_last_logged_opid) const {
> warning: the parameter 'tombstone_last_logged_opid' is copied for each invo

PS4, Line 2515: (!tombstone_last_logged_opid
> not 100% following why this portion of the condition is necessary
if we are voting while tombstoned then we don't have to be running

PS4, Line 2520: state_ == kShutdown
> should we instead be checking the expected state (ie kStopped)? aren't ther
We can tombstoned vote in kInit and kStopped state, even in kRunning actually 
due to a race between the check in TabletService and execution in 
RaftConsensus. It's not possible to expose RaftConsensus in kNew state due to 
code in TabletReplica however it would be better to encapsulate that guarantee 
within this class so I'll add a Create() factory method.

Line 2634:   return kMinimumTerm;
> under what circumstances do we hit this case? It seems like cmeta_ should b
Good catch; this was left over from a previous attempt on this patch and is not 
possible with the current rev. However I agree with you that a factory method 
for RaftConsensus would be preferable here.

Line 2685:   if (cmeta_) {
> same

File src/kudu/integration-tests/cluster_itest_util.h:

Line 237:                    const int64_t candidate_term,
> warning: parameter 'candidate_term' is const-qualified in the function decl

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 def
I thought it was nice to pass them as optional so that if you don't specify 
them you get the default behavior as specified in the proto file.

File src/kudu/integration-tests/tombstoned_voting-stress-test.cc:

Line 38: using kudu::consensus::MakeOpId;
> warning: using decl 'MakeOpId' is unused [misc-unused-using-decls]

Line 39: using kudu::consensus::LeaderStepDownResponsePB;
> warning: using decl 'LeaderStepDownResponsePB' is unused [misc-unused-using

Line 41: using kudu::consensus::RaftConsensus;
> warning: using decl 'RaftConsensus' is unused [misc-unused-using-decls]

Line 42: using kudu::consensus::RaftPeerPB;
> warning: using decl 'RaftPeerPB' is unused [misc-unused-using-decls]

Line 47: using kudu::tablet::TabletStatePB;
> warning: using decl 'TabletStatePB' is unused [misc-unused-using-decls]

Line 48: using kudu::tserver::TabletServerErrorPB;
> warning: using decl 'TabletServerErrorPB' is unused [misc-unused-using-decl

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 o
Based on a grep through the code, the master only knows about tablet::RUNNING 
and tablet::FAILED so it shouldn't even notice.

File src/kudu/tablet/tablet_metadata.h:

Line 216:   void SetPreFlushCallback(StatusClosure callback) { 
pre_flush_callback_ = callback; }
> warning: parameter 'callback' is passed by value and only copied once; cons

File src/kudu/tablet/tablet_replica.cc:

Line 133:   set_state(INITIALIZED);
> maybe moot based on the suggestion of a factory method, but if a method lik

Line 247:   // TODO: KUDU-183: Keep track of the pending tasks and send an 
"abort" message.
> warning: missing username/bug in TODO [google-readability-todo]

Line 272:     // Release mem tracker resources.
> is this comment stale?
Maybe stale? Either way it seems pretty irrelevant. I'll remove it.

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

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 
The problem with a monolithic construction and initialization of a 
TabletReplica is that it prevents replicas in certain failure states from being 
visible to the master or the web UI.

For example, replicas that fail to Init() at startup time due to a missing or 
corrupted cmeta should still be reported on the web UI and to the master -- at 
least, I'm pretty sure that's what we want. I suppose an alternative would be 
to automatically tombstone a tablet that failed to start up but that seems 
awfully aggressive, especially in the case where there is only 1 replica per 

PS4, Line 96: tombstoned 
> voting in general, right? ie what would happen if you called Stop() but the
The log would be closed so we wouldn't be able to vote on a 
stopped-but-not-tombstoned replica.

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/tablet_service.cc:

Line 136: using kudu::tablet::TabletStatusPB;
> warning: using decl 'TabletStatusPB' is unused [misc-unused-using-decls]

Line 947:   tablet::TabletDataState data_state = 
> aren't all these state checks racy? after you grab this here, there's nothi
Yeah, it's racy, but it's safe because we're checking after we have a reference 
to the replica, and the replica can only have one consensus object, which will 
be shut down if the tablet starts copying again, for example.

I'll add a block comment here to explain; LMK what you think.

I plan to move the last-logged opid from TabletMetadata to ConsensusMetadata in 
a follow-up change.

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

That's right.

> Would it be safe to just pass this in always as a non-optional, and then in 
> RequestVote always just take max(log_->GetLastLoggedOpId(), 
> tombstone_last_logged_opid())?

I don't think so. The case I am worried about is if a tablet fails to start up, 
so it doesn't have a log running, and then it gets tombstoned by the master. In 
that case we will not currently store the last-logged opid at DeleteTablet() 
time. So if we don't know what our last-logged opid was (even though we did 
actually have one) then I don't think it's safe for us to simply start voting 
based on a last-logged opid of (0,0).

Additionally, just using max() seems wrong in case there was a log truncation 
involved. We want to use our latest opid state.

File src/kudu/tserver/tserver-path-handlers.cc:

Line 460:   if (!consensus || !consensus->IsRunning()) {
> instead of this, can you make Consensus::DumpStatusHtml dump something reas
good catch

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: 4
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