Todd Lipcon has posted comments on this change. Change subject: KUDU-871. Support tombstoned voting ......................................................................
Patch Set 6: (25 comments) http://gerrit.cloudera.org:8080/#/c/6960/6/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: PS6, Line 1485: && state_ == kRunning why is this condition necesary? if we've just been tombstoned it still makes sense that we should withhold votes for a period of time, right? PS6, Line 1509: CheckSafeToVoteUnlocked the duplicate logic here with CheckSafeToVoteUnlocked is a little unnerving. What do you think about essentially inlining its logic here and merging with this if condition? eg something of this nature: switch (state_) { case kShutdown: return IllegalState... case kRunning: local_last_logged = GetLatestFromLog() break; default: if (FLAGS_allow_tombstone_voting && tombstone_last_logged_opid) { local_last_logged = tombstone_last_logged; } else { return IllegalState } break; } I think that might be easier to follow and net less lines of code PS6, Line 1514: DCHECK(tombstone_last_logged_opid) << LogPrefixUnlocked() << "expected last-logged opid"; : local_last_logged_opid = *tombstone_last_logged_opid; if this DCHECK fails, would we crash anyway trying to dereference the boost::none? if so, I think we should either use CHECK (so we get a nice crash log) or we should try to handle this case by just denying the vote (if we aren't 100% sure that this would always be the case, we can always vote conservatively) http://gerrit.cloudera.org:8080/#/c/6960/6/src/kudu/consensus/raft_consensus.h File src/kudu/consensus/raft_consensus.h: Line 130: RaftConsensus(ConsensusOptions options, can the ctor be made private? (may need ALLOW_MAKE_SHARED) I don't see any external usage of it but maybe my grepping was bad. PS6, Line 271: term > nit: current_term() would be more explicit I think CurrentTerm or Term would be better since this takes lock_ and that lock is often held for long periods of time (thus the call may be quite slow compared to your typical getter) PS6, Line 292: Tombstoned voting > not super clear what "tombstoned voting" is maybe explain it somewhere? agreed. Maybe better here to just say that it synchronously transitions to kStopped state, and refer to the kStopped definition in the state enum for what behavior that entails? Line 297: void Shutdown(); same Line 339: enum State { it would be nice to have a state transition diagram of sorts here. Or alternatively, for each state, list out the potential subsequent states and what triggers the transition? eg I can't remember now whether we can transition back from kStopped to kRunning or not. PS6, Line 340: // RaftConsensus has been freshly constructed. : kNew, is this state ever externally exposed, considering we now have the factory method that returns a post-init object? if not, worth noting as much PS6, Line 343: // RaftConsensus has been initialized. : kInitialized, in terms of behavior, how is this state different than Stopped? eg let's say I start up a server which has some tablets in tombstoned state, do their RaftConsensus instances end up in kInitialized state or kStopped state? Is it important to make a distinction? PS6, Line 391: // Initializes the RaftConsensus object. This should be called before : // publishing this object to any thread other than the thread that invoked : // the constructor. : Status Init(); since the constructor is only invoked by the factory method, maybe not necessary to be so explicit? PS6, Line 647: // existence of a known last logged opid. : Status CheckSafeToVoteUnlocked(const boost::optional<OpId>& tombstone_last_logged_opid) : similar to David's comment on the implementation: I think it would make more sense to pass a 'bool tombstoned_op_id_known' or somesuch, and in the implementation add a comment explaining the scenario(s) in which we would _not_ know the tombstoned op id and why it's not safe to vote in those scenarios. Use of a bool instead of the opid makes it more clear that we just care about whether we know a valid opid, and aren't deciding here based on its value. Line 783: AtomicBool stopped_; it seems it was already like this, but what's the distinction between this 'stopped_' member and just checking 'state_ == kStopped'? mind adding a comment explaining and perhaps a TODO to get rid of it if possible? (not going to make you get rid of it in this patch since it's pre-existing and I dont think you made it worse) 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 80: call Stop() on you mean 'manually tombstone' right? I was confused by reading this comment into thinking you were SIGSTOPping TS1 and then expecting it to vote or something, which didn't make much sense. http://gerrit.cloudera.org:8080/#/c/6960/4/src/kudu/integration-tests/tombstoned_voting-stress-test.cc File src/kudu/integration-tests/tombstoned_voting-stress-test.cc: Line 60: protected: this seems unused? Line 182: // We want to control leader election manually and we only want 2 replicas. would be nice to have another more stressful test that doesn't attempt to verify any kind of correctness or coordinate the lifecycle changes with the vote requests. I think the coordination here has some effect that reduces the probability of races http://gerrit.cloudera.org:8080/#/c/6960/6/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: PS6, Line 142: consensus_ = std::move(consensus); : set_state(INITIALIZED); : i'm surprised these lines don't need to take lock_? is this method always called before publishing a TabletReplica instance? Line 315: case STOPPED: hrm, no case for STOPPING? http://gerrit.cloudera.org:8080/#/c/6960/6/src/kudu/tablet/tablet_replica.h File src/kudu/tablet/tablet_replica.h: PS6, Line 81: // Initializes RaftConsensus. : Status Init(ThreadPool* raft_pool); left a similar question in the implementation: is it expected that this be called before publishing an instance? might also be nice to add a note that, even if this fails, it leaves the instance in a self-consistent state (which is in contrast to some other Init methods) so that it can be left registered in the replica map, etc? PS6, Line 96: tombstoned voting I think it's better to generalize this a bit. Is this _only_ used in the case of tombstoning? or can all stopped replicas vote? is it legal to call this without first putting the tablet into tombstoned state? Line 102: // Shut down this tablet replica permanently. does this require that Stop be called first? again would be nice to have some diagram of the state transitions to refer to for these lifecycle methods, perhaps in the class doc http://gerrit.cloudera.org:8080/#/c/6960/4/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: Line 947: // from the TabletMetadata into RaftConsensus::RequestVote(). > Yeah, it's racy, but it's safe because we're checking after we have a refer I guess my concern is less with the safety of the race but more with whether the state checks are even proper, given they are race. I did some hacking on the stress test locally to inject some sleeps after grabbing the state and managed to catch the case where the tablet transitioned from READY to TOMBSTONED. This resulted in a call into RaftConsensus::RequestVote() with no last_logged_opid, so it returned: > Illegal state: RaftConsensus is not running: State = Stopped given we already need to handle this racy check within the RaftConsensus vote code itself, I'm not sure what extra value this is bringing to check it additionally here? BTW, it would be really nice to include some class-level lifecycle comments somewhere. I keep forgetting what happens to the TabletReplica, Tablet, TabletMetadata, and RaftConsensus instances in the case of a tombstone and a re-copy -- which ones are reused, how we ensure that the old ones aren't used anymore after the new ones are created, etc. I remember drawing such a diagram with you a couple weeks ago, and would be nice to have it in the code for reference. PS4, Line 966: LOG(INFO) << "Received RequestConsensusVote() RPC: " << Secu > > when would this equal MinimumOpId? if it's never been tombstoned? > Additionally, just using max() seems wrong in case there was a log truncation > involved. We want to use our latest opid state. If there is truncation then the truncating OpId would be 'higher' than the other one, because it has a higher term, right? i.e comparison is lexicographic comparison of <term, index> tuples http://gerrit.cloudera.org:8080/#/c/6960/6/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: PS6, Line 979: replica->tablet_metadata()->tombstone_last_logged_opid maybe cleaner for it to just return an optional<OpId> itself rather than the special uninitialized value? http://gerrit.cloudera.org:8080/#/c/6960/6/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: Line 663: Status s = DeleteTabletData(replica->tablet_metadata(), cmeta_manager_, delete_type, per discussion on Slack, we may need to modify this so that, if there was no valid log prior to the deletion, we explicitly _clear_ the last_logged_opid, or set to some sentinel value or something to indicate that it's not safe to vote due to potentially lost log state -- 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
