raft_consensus: clean up overrides, protected methods RaftConsensus used to have a subclass mock in raft_consensus-test, but that test is now removed. Given this, we can make some previously-protected methods private and non-virtual.
I also took this opportunity to modernize the 'override' style to C++11, and also to remove a function which was essentially duplicate code. Change-Id: Ib52d3ac40ed68a4ff8b1738897f6ef62f94a843b Reviewed-on: http://gerrit.cloudera.org:8080/4620 Tested-by: Kudu Jenkins Reviewed-by: Todd Lipcon <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/bda9b94e Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/bda9b94e Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/bda9b94e Branch: refs/heads/master Commit: bda9b94e88b2602dcc029e763c8a9e0eecff8e08 Parents: 4bcbb4a Author: Todd Lipcon <[email protected]> Authored: Tue Oct 4 14:34:10 2016 -0700 Committer: Todd Lipcon <[email protected]> Committed: Wed Oct 5 21:41:36 2016 +0000 ---------------------------------------------------------------------- src/kudu/consensus/raft_consensus.cc | 9 +- src/kudu/consensus/raft_consensus.h | 98 +++++++++----------- .../consensus/raft_consensus_quorum-test.cc | 2 +- 3 files changed, 45 insertions(+), 64 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/bda9b94e/src/kudu/consensus/raft_consensus.cc ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/raft_consensus.cc b/src/kudu/consensus/raft_consensus.cc index 0cb724e..3e4a89b 100644 --- a/src/kudu/consensus/raft_consensus.cc +++ b/src/kudu/consensus/raft_consensus.cc @@ -1511,12 +1511,6 @@ void RaftConsensus::Shutdown() { shutdown_.Store(true, kMemOrderRelease); } -RaftPeerPB::Role RaftConsensus::GetActiveRole() const { - ReplicaState::UniqueLock lock; - CHECK_OK(state_->LockForRead(&lock)); - return state_->GetActiveRoleUnlocked(); -} - OpId RaftConsensus::GetLatestOpIdFromLog() { OpId id; log_->GetLatestEntryOpId(&id); @@ -1675,8 +1669,7 @@ Status RaftConsensus::RequestVoteRespondVoteGranted(const VoteRequestPB* request RaftPeerPB::Role RaftConsensus::role() const { ReplicaState::UniqueLock lock; CHECK_OK(state_->LockForRead(&lock)); - return GetConsensusRole(state_->GetPeerUuid(), - state_->ConsensusStateUnlocked(CONSENSUS_CONFIG_ACTIVE)); + return state_->GetActiveRoleUnlocked(); } std::string RaftConsensus::LogPrefixUnlocked() { http://git-wip-us.apache.org/repos/asf/kudu/blob/bda9b94e/src/kudu/consensus/raft_consensus.h ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/raft_consensus.h b/src/kudu/consensus/raft_consensus.h index 887201b..07ef186 100644 --- a/src/kudu/consensus/raft_consensus.h +++ b/src/kudu/consensus/raft_consensus.h @@ -91,69 +91,64 @@ class RaftConsensus : public Consensus, virtual ~RaftConsensus(); - virtual Status Start(const ConsensusBootstrapInfo& info) OVERRIDE; + Status Start(const ConsensusBootstrapInfo& info) override; - virtual bool IsRunning() const OVERRIDE; + bool IsRunning() const override; // Emulates an election by increasing the term number and asserting leadership // in the configuration by sending a NO_OP to other peers. // This is NOT safe to use in a distributed configuration with failure detection // enabled, as it could result in a split-brain scenario. - virtual Status EmulateElection() OVERRIDE; + Status EmulateElection() override; - virtual Status StartElection(ElectionMode mode) OVERRIDE; + Status StartElection(ElectionMode mode) override; - virtual Status WaitUntilLeaderForTests(const MonoDelta& timeout) OVERRIDE; + Status WaitUntilLeaderForTests(const MonoDelta& timeout) override; - virtual Status StepDown(LeaderStepDownResponsePB* resp) OVERRIDE; + Status StepDown(LeaderStepDownResponsePB* resp) override; // Call StartElection(), log a warning if the call fails (usually due to // being shut down). void ReportFailureDetected(const std::string& name, const Status& msg); - virtual Status Replicate(const scoped_refptr<ConsensusRound>& round) OVERRIDE; + Status Replicate(const scoped_refptr<ConsensusRound>& round) override; - virtual Status CheckLeadershipAndBindTerm(const scoped_refptr<ConsensusRound>& round) OVERRIDE; + Status CheckLeadershipAndBindTerm(const scoped_refptr<ConsensusRound>& round) override; - virtual Status Update(const ConsensusRequestPB* request, - ConsensusResponsePB* response) OVERRIDE; + Status Update(const ConsensusRequestPB* request, + ConsensusResponsePB* response) override; - virtual Status RequestVote(const VoteRequestPB* request, - VoteResponsePB* response) OVERRIDE; + Status RequestVote(const VoteRequestPB* request, + VoteResponsePB* response) override; - virtual Status ChangeConfig(const ChangeConfigRequestPB& req, - const StatusCallback& client_cb, - boost::optional<tserver::TabletServerErrorPB::Code>* error_code) - OVERRIDE; + Status ChangeConfig(const ChangeConfigRequestPB& req, + const StatusCallback& client_cb, + boost::optional<tserver::TabletServerErrorPB::Code>* error_code) override; - virtual RaftPeerPB::Role role() const OVERRIDE; + Status GetLastOpId(OpIdType type, OpId* id) override; - virtual std::string peer_uuid() const OVERRIDE; + RaftPeerPB::Role role() const override; - virtual std::string tablet_id() const OVERRIDE; + std::string peer_uuid() const override; - virtual ConsensusStatePB ConsensusState(ConsensusConfigType type) const OVERRIDE; + std::string tablet_id() const override; - virtual RaftConfigPB CommittedConfig() const OVERRIDE; + ConsensusStatePB ConsensusState(ConsensusConfigType type) const override; - virtual void DumpStatusHtml(std::ostream& out) const OVERRIDE; + RaftConfigPB CommittedConfig() const override; - virtual void Shutdown() OVERRIDE; + void DumpStatusHtml(std::ostream& out) const override; - // Makes this peer advance it's term (and step down if leader), for tests. - virtual Status AdvanceTermForTests(int64_t new_term); + void Shutdown() override; - // Return the active (as opposed to committed) role. - RaftPeerPB::Role GetActiveRole() const; + // Makes this peer advance it's term (and step down if leader), for tests. + Status AdvanceTermForTests(int64_t new_term); // Returns the replica state for tests. This should never be used outside of // tests, in particular calling the LockFor* methods on the returned object // can cause consensus to deadlock. ReplicaState* GetReplicaStateForTests(); - virtual Status GetLastOpId(OpIdType type, OpId* id) OVERRIDE; - - //------------------------------------------------------------ // PeerMessageQueueObserver implementation //------------------------------------------------------------ @@ -171,25 +166,6 @@ class RaftConsensus : public Consensus, log::RetentionIndexes GetRetentionIndexes() override; - protected: - // Trigger that a non-Transaction ConsensusRound has finished replication. - // If the replication was successful, an status will be OK. Otherwise, it - // may be Aborted or some other error status. - // If 'status' is OK, write a Commit message to the local WAL based on the - // type of message it is. - // The 'client_cb' will be invoked at the end of this execution. - virtual void NonTxRoundReplicationFinished(ConsensusRound* round, - const StatusCallback& client_cb, - const Status& status); - - // As a leader, append a new ConsensusRound to the queue. - // Only virtual and protected for mocking purposes. - virtual Status AppendNewRoundToQueueUnlocked(const scoped_refptr<ConsensusRound>& round); - - // As a follower, start a consensus round not associated with a Transaction. - // Only virtual and protected for mocking purposes. - virtual Status StartConsensusOnlyRoundUnlocked(const ReplicateRefPtr& msg); - private: friend class ReplicaState; friend class RaftConsensusQuorumTest; @@ -288,12 +264,6 @@ class RaftConsensus : public Consensus, // and also truncate the LogCache accordingly. void TruncateAndAbortOpsAfterUnlocked(int64_t truncate_after_index); - // Pushes a new Raft configuration to a majority of peers. Contrary to write operations, - // this actually waits for the commit round to reach a majority of peers, so that we know - // we can proceed. If this returns Status::OK(), a majority of peers have accepted the new - // configuration. The peer cannot perform any additional operations until this succeeds. - Status PushConfigurationToPeersUnlocked(const RaftConfigPB& new_config); - // Returns the most recent OpId written to the Log. OpId GetLatestOpIdFromLog(); @@ -432,6 +402,24 @@ class RaftConsensus : public Consensus, const RaftConfigPB& committed_config, const std::string& reason); + // Trigger that a non-Transaction ConsensusRound has finished replication. + // If the replication was successful, an status will be OK. Otherwise, it + // may be Aborted or some other error status. + // If 'status' is OK, write a Commit message to the local WAL based on the + // type of message it is. + // The 'client_cb' will be invoked at the end of this execution. + void NonTxRoundReplicationFinished(ConsensusRound* round, + const StatusCallback& client_cb, + const Status& status); + + // As a leader, append a new ConsensusRound to the queue. + // Only virtual and protected for mocking purposes. + Status AppendNewRoundToQueueUnlocked(const scoped_refptr<ConsensusRound>& round); + + // As a follower, start a consensus round not associated with a Transaction. + // Only virtual and protected for mocking purposes. + Status StartConsensusOnlyRoundUnlocked(const ReplicateRefPtr& msg); + // Threadpool for constructing requests to peers, handling RPC callbacks, // etc. gscoped_ptr<ThreadPool> thread_pool_; http://git-wip-us.apache.org/repos/asf/kudu/blob/bda9b94e/src/kudu/consensus/raft_consensus_quorum-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/consensus/raft_consensus_quorum-test.cc b/src/kudu/consensus/raft_consensus_quorum-test.cc index 9d63266..362a230 100644 --- a/src/kudu/consensus/raft_consensus_quorum-test.cc +++ b/src/kudu/consensus/raft_consensus_quorum-test.cc @@ -78,7 +78,7 @@ void DoNothing(const string& s) { Status WaitUntilLeaderForTests(RaftConsensus* raft) { MonoTime deadline = MonoTime::Now() + MonoDelta::FromSeconds(15); while (MonoTime::Now() < deadline) { - if (raft->GetActiveRole() == RaftPeerPB::LEADER) { + if (raft->role() == RaftPeerPB::LEADER) { return Status::OK(); } SleepFor(MonoDelta::FromMilliseconds(10));
