You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by mp...@apache.org on 2018/02/05 19:50:18 UTC
kudu git commit: Don't rely on pending config OpId index for peer
promotion
Repository: kudu
Updated Branches:
refs/heads/master c19a42170 -> 839e3b3a6
Don't rely on pending config OpId index for peer promotion
As part of the effort to make OpId and timestamp assignment atomic
we can't rely on a pending config having been assigned an OpId anymore.
This because the config is marked as the active config _before_ an OpId
is assigned later on, by the queue. The OpId only actually gets set when
the config is committed. Follow up patches will complete remove this
reliance.
In general this doesn't have much impact except where we're promoting a peer.
In this case we currently pass the current active config's term and index from
the queue back to consensus which then uses this info to perform the promotion
only when the current term and index match. We can't do this in the new
setting where the term and index are not available, but moreover this info
is not strictly needed for promotion at all, so this patch removes this
extra info being passed around.
The reasoning why the extra info is not needed is the following:
- If a peer' uuid is marked for promotion in the current committed
config, as verified directly through cmeta, and is considered up-to-date
enough to be promoted, it doesn't matter whether the active config in the
queue is the same as the committed config. The peer still can and should
be promoted.
- Essentially, omitting this extra info just means that we have to re-check
that the member type and attributes are still the expected ones, which is
not costly.
Change-Id: I3a033ce16f626458efdb59b1b4dd8450e8591761
Reviewed-on: http://gerrit.cloudera.org:8080/9161
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Mike Percy <mp...@apache.org>
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/839e3b3a
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/839e3b3a
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/839e3b3a
Branch: refs/heads/master
Commit: 839e3b3a67619bc327801e128293347fb59e1091
Parents: c19a421
Author: David Alves <dr...@apache.org>
Authored: Tue Jan 30 09:40:42 2018 -0800
Committer: Mike Percy <mp...@apache.org>
Committed: Mon Feb 5 19:49:32 2018 +0000
----------------------------------------------------------------------
src/kudu/consensus/consensus_queue.cc | 15 ++-------
src/kudu/consensus/consensus_queue.h | 8 ++---
src/kudu/consensus/raft_consensus.cc | 54 ++++++++++++++----------------
src/kudu/consensus/raft_consensus.h | 11 ++----
4 files changed, 34 insertions(+), 54 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/839e3b3a/src/kudu/consensus/consensus_queue.cc
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/consensus_queue.cc b/src/kudu/consensus/consensus_queue.cc
index 1889ed3..c53ad1d 100644
--- a/src/kudu/consensus/consensus_queue.cc
+++ b/src/kudu/consensus/consensus_queue.cc
@@ -972,14 +972,7 @@ void PeerMessageQueue::ResponseFromPeer(const std::string& peer_uuid,
//
// TODO(mpercy): Should we introduce a function SafeToPromote() that
// does the same calculation as SafeToEvict() but for adding a VOTER?
- //
- // Note: we can pass in the active config's 'opid_index' as the
- // committed config's opid_index because if we're in the middle of a
- // config change, this requested config change will be rejected
- // anyway.
- NotifyObserversOfPeerToPromote(peer->uuid(),
- queue_state_.current_term,
- queue_state_.active_config->opid_index());
+ NotifyObserversOfPeerToPromote(peer->uuid());
}
}
@@ -1303,13 +1296,11 @@ void PeerMessageQueue::NotifyObserversOfFailedFollower(const string& uuid,
LogPrefixUnlocked() + "Unable to notify RaftConsensus of abandoned follower.");
}
-void PeerMessageQueue::NotifyObserversOfPeerToPromote(const string& peer_uuid,
- int64_t term,
- int64_t committed_config_opid_index) {
+void PeerMessageQueue::NotifyObserversOfPeerToPromote(const string& peer_uuid) {
WARN_NOT_OK(raft_pool_observers_token_->SubmitClosure(
Bind(&PeerMessageQueue::NotifyObserversTask, Unretained(this),
[=](PeerMessageQueueObserver* observer) {
- observer->NotifyPeerToPromote(peer_uuid, term, committed_config_opid_index);
+ observer->NotifyPeerToPromote(peer_uuid);
})),
LogPrefixUnlocked() + "Unable to notify RaftConsensus of peer to promote.");
}
http://git-wip-us.apache.org/repos/asf/kudu/blob/839e3b3a/src/kudu/consensus/consensus_queue.h
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/consensus_queue.h b/src/kudu/consensus/consensus_queue.h
index 2a6df6c..c64ae86 100644
--- a/src/kudu/consensus/consensus_queue.h
+++ b/src/kudu/consensus/consensus_queue.h
@@ -458,9 +458,7 @@ class PeerMessageQueue {
void NotifyObserversOfFailedFollower(const std::string& uuid,
int64_t term,
const std::string& reason);
- void NotifyObserversOfPeerToPromote(const std::string& peer_uuid,
- int64_t term,
- int64_t committed_config_opid_index);
+ void NotifyObserversOfPeerToPromote(const std::string& peer_uuid);
void NotifyObserversOfPeerHealthChange();
// Notify all PeerMessageQueueObservers using the given callback function.
@@ -565,9 +563,7 @@ class PeerMessageQueueObserver {
// Notify the observer that the specified peer is ready to be promoted from
// NON_VOTER to VOTER.
- virtual void NotifyPeerToPromote(const std::string& peer_uuid,
- int64_t term,
- int64_t committed_config_opid_index) = 0;
+ virtual void NotifyPeerToPromote(const std::string& peer_uuid) = 0;
// Notify the observer that the health of one of the peers has changed.
virtual void NotifyPeerHealthChange() = 0;
http://git-wip-us.apache.org/repos/asf/kudu/blob/839e3b3a/src/kudu/consensus/raft_consensus.cc
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/raft_consensus.cc b/src/kudu/consensus/raft_consensus.cc
index d673d40..2040733 100644
--- a/src/kudu/consensus/raft_consensus.cc
+++ b/src/kudu/consensus/raft_consensus.cc
@@ -789,15 +789,11 @@ void RaftConsensus::NotifyFailedFollower(const string& uuid,
LogPrefixThreadSafe() + "Unable to start TryRemoveFollowerTask");
}
-void RaftConsensus::NotifyPeerToPromote(const std::string& peer_uuid,
- int64_t term,
- int64_t committed_config_opid_index) {
+void RaftConsensus::NotifyPeerToPromote(const std::string& peer_uuid) {
// Run the config change on the raft thread pool.
WARN_NOT_OK(raft_pool_token_->SubmitFunc(std::bind(&RaftConsensus::TryPromoteNonVoterTask,
shared_from_this(),
- peer_uuid,
- term,
- committed_config_opid_index)),
+ peer_uuid)),
LogPrefixThreadSafe() + "Unable to start TryPromoteNonVoterTask");
}
@@ -821,38 +817,40 @@ void RaftConsensus::TryRemoveFollowerTask(const string& uuid,
LogPrefixThreadSafe() + "Unable to remove follower " + uuid);
}
-void RaftConsensus::TryPromoteNonVoterTask(const std::string& peer_uuid,
- int64_t term,
- int64_t committed_config_opid_index) {
+void RaftConsensus::TryPromoteNonVoterTask(const std::string& peer_uuid) {
string msg = Substitute("attempt to promote peer $0: ", peer_uuid);
+ int64_t current_committed_config_index;
{
ThreadRestrictions::AssertWaitAllowed();
LockGuard l(lock_);
- int64_t current_term = CurrentTermUnlocked();
- if (term != current_term) {
- LOG_WITH_PREFIX_UNLOCKED(INFO) << msg << "requested in "
- << "previous term " << term
- << ", but a leader election "
- << "likely occurred since then. Doing nothing.";
+
+ if (cmeta_->has_pending_config()) {
+ LOG_WITH_PREFIX_UNLOCKED(INFO) << msg << "there is already a config change operation "
+ << "in progress. Unable to promote follower until it "
+ << "completes. Doing nothing.";
return;
}
- int64_t current_committed_config_opid_index =
- cmeta_->GetConfigOpIdIndex(COMMITTED_CONFIG);
- if (committed_config_opid_index != current_committed_config_opid_index) {
- LOG_WITH_PREFIX_UNLOCKED(INFO) << msg << "notified when "
- << "committed config had opid index "
- << committed_config_opid_index << ", but now "
- << "the committed config has opid index "
- << current_committed_config_opid_index
+ // Check if the peer is still part of the current committed config.
+ RaftConfigPB committed_config = cmeta_->CommittedConfig();
+ current_committed_config_index = committed_config.opid_index();
+
+ RaftPeerPB* peer_pb;
+ Status s = GetRaftConfigMember(&committed_config, peer_uuid, &peer_pb);
+ if (!s.ok()) {
+ LOG_WITH_PREFIX_UNLOCKED(INFO) << msg << "can't find peer in the "
+ << "current committed config: "
+ << committed_config.ShortDebugString()
<< ". Doing nothing.";
return;
}
- if (cmeta_->has_pending_config()) {
- LOG_WITH_PREFIX_UNLOCKED(INFO) << msg << "there is already a config change operation "
- << "in progress. Unable to promote follower until it "
- << "completes. Doing nothing.";
+ // Also check if the peer it still a NON_VOTER waiting for promotion.
+ if (peer_pb->member_type() != RaftPeerPB::NON_VOTER || !peer_pb->attrs().promote()) {
+ LOG_WITH_PREFIX_UNLOCKED(INFO) << msg << "peer is either no longer a NON_VOTER "
+ << "or not marked for promotion anymore. Current "
+ << "config: " << committed_config.ShortDebugString()
+ << ". Doing nothing.";
return;
}
}
@@ -863,7 +861,7 @@ void RaftConsensus::TryPromoteNonVoterTask(const std::string& peer_uuid,
req.mutable_server()->set_permanent_uuid(peer_uuid);
req.mutable_server()->set_member_type(RaftPeerPB::VOTER);
req.mutable_server()->mutable_attrs()->set_promote(false);
- req.set_cas_config_opid_index(committed_config_opid_index);
+ req.set_cas_config_opid_index(current_committed_config_index);
LOG(INFO) << LogPrefixThreadSafe() << "attempting to promote NON_VOTER "
<< peer_uuid << " to VOTER";
boost::optional<TabletServerErrorPB::Code> error_code;
http://git-wip-us.apache.org/repos/asf/kudu/blob/839e3b3a/src/kudu/consensus/raft_consensus.h
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/raft_consensus.h b/src/kudu/consensus/raft_consensus.h
index 5ace791..81aa0a7 100644
--- a/src/kudu/consensus/raft_consensus.h
+++ b/src/kudu/consensus/raft_consensus.h
@@ -335,9 +335,7 @@ class RaftConsensus : public std::enable_shared_from_this<RaftConsensus>,
int64_t term,
const std::string& reason) override;
- void NotifyPeerToPromote(const std::string& peer_uuid,
- int64_t term,
- int64_t committed_config_opid_index) override;
+ void NotifyPeerToPromote(const std::string& peer_uuid) override;
void NotifyPeerHealthChange() override;
@@ -650,11 +648,8 @@ class RaftConsensus : public std::enable_shared_from_this<RaftConsensus>,
const RaftConfigPB& committed_config,
const std::string& reason);
- // Attempt to promote the given non-voter to a voter, if the 'term'
- // and 'committed_config_opid_index' CAS parameters are both still current.
- void TryPromoteNonVoterTask(const std::string& peer_uuid,
- int64_t term,
- int64_t committed_config_opid_index);
+ // Attempt to promote the given non-voter to a voter.
+ void TryPromoteNonVoterTask(const std::string& peer_uuid);
// Called when the failure detector expires.
// Submits ReportFailureDetectedTask() to a thread pool.