Todd Lipcon has posted comments on this change. Change subject: KUDU-871. Support tombstoned voting ......................................................................
Patch Set 4: (21 comments) did a first pass, probably will have more comments after a second pass but figured I'd send these along http://gerrit.cloudera.org:8080/#/c/6960/4/src/kudu/consensus/raft_consensus.cc 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 broadened a bit now PS4, Line 1493: tombstone_last_logged_opid ? *tombstone_last_logged_opid : : GetLatestOpIdFromLog(); 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_) { same http://gerrit.cloudera.org:8080/#/c/6960/4/src/kudu/integration-tests/cluster_itest_util.h 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? http://gerrit.cloudera.org:8080/#/c/6960/4/src/kudu/tablet/metadata.proto 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? http://gerrit.cloudera.org:8080/#/c/6960/4/src/kudu/tablet/tablet_replica.cc 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 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_ : << ": " << SecureDebugString(*status_pb_out); leftover debug print? Line 427: CHECK_EQ(INITIALIZED, state_); redundant with the DCHECK in set_state http://gerrit.cloudera.org:8080/#/c/6960/4/src/kudu/tablet/tablet_replica.h 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)> mark_dirty_clbk); : : // 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? eg: 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. or SHUTDOWN http://gerrit.cloudera.org:8080/#/c/6960/4/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: Line 947: tablet::TabletDataState data_state = replica->tablet_metadata()->tablet_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(), tombstone_last_logged_opid())? http://gerrit.cloudera.org:8080/#/c/6960/4/src/kudu/tserver/tserver-path-handlers.cc File src/kudu/tserver/tserver-path-handlers.cc: 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 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: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
