You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by ad...@apache.org on 2018/07/06 18:20:22 UTC
kudu git commit: consensus: release lock before doing DNS lookups in
leader elections
Repository: kudu
Updated Branches:
refs/heads/master 334ecafd0 -> e157663ba
consensus: release lock before doing DNS lookups in leader elections
Noticed this while poring over a ~60k thread pstack dump from an overloaded
cluster. Ignoring for a minute that the consensus lock isn't actually needed
to serialize this work, taking a spinlock during a potentially blocking
operation is a no-no. As is doing substantive work in a class constructor;
that's a violation of POLA [1] in our codebase.
1. https://en.wikipedia.org/wiki/Principle_of_least_astonishment
Change-Id: I8403ed74d3199ef90779b86bb7d9727a3c4af876
Reviewed-on: http://gerrit.cloudera.org:8080/10879
Reviewed-by: Todd Lipcon <to...@apache.org>
Tested-by: Adar Dembo <ad...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/e157663b
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/e157663b
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/e157663b
Branch: refs/heads/master
Commit: e157663ba7cbe1ba6cc904dbaa76e97f8c6e31cf
Parents: 334ecaf
Author: Adar Dembo <ad...@cloudera.com>
Authored: Thu Jul 5 21:43:26 2018 -0700
Committer: Adar Dembo <ad...@cloudera.com>
Committed: Fri Jul 6 18:20:01 2018 +0000
----------------------------------------------------------------------
src/kudu/consensus/leader_election.cc | 52 +++++++++++++++++-------------
src/kudu/consensus/leader_election.h | 25 +++++++-------
src/kudu/consensus/raft_consensus.cc | 6 ++--
3 files changed, 48 insertions(+), 35 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/e157663b/src/kudu/consensus/leader_election.cc
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/leader_election.cc b/src/kudu/consensus/leader_election.cc
index 0590fdf..bd2c8a9 100644
--- a/src/kudu/consensus/leader_election.cc
+++ b/src/kudu/consensus/leader_election.cc
@@ -21,6 +21,7 @@
#include <mutex>
#include <ostream>
#include <type_traits>
+#include <vector>
#include <boost/bind.hpp> // IWYU pragma: keep
#include <glog/logging.h>
@@ -45,6 +46,7 @@ namespace kudu {
namespace consensus {
using std::string;
+using std::vector;
using strings::Substitute;
///////////////////////////////////////////////////
@@ -159,35 +161,51 @@ string LeaderElection::VoterState::PeerInfo() const {
// LeaderElection
///////////////////////////////////////////////////
-LeaderElection::LeaderElection(const RaftConfigPB& config,
+LeaderElection::LeaderElection(RaftConfigPB config,
PeerProxyFactory* proxy_factory,
- const VoteRequestPB& request,
+ VoteRequestPB request,
gscoped_ptr<VoteCounter> vote_counter,
MonoDelta timeout,
ElectionDecisionCallback decision_callback)
: has_responded_(false),
- request_(request),
+ config_(std::move(config)),
+ proxy_factory_(proxy_factory),
+ request_(std::move(request)),
vote_counter_(std::move(vote_counter)),
timeout_(timeout),
decision_callback_(std::move(decision_callback)),
highest_voter_term_(0) {
- for (const RaftPeerPB& peer : config.peers()) {
- if (request.candidate_uuid() == peer.permanent_uuid()) {
+}
+
+LeaderElection::~LeaderElection() {
+ std::lock_guard<Lock> guard(lock_);
+ DCHECK(has_responded_); // We must always call the callback exactly once.
+ STLDeleteValues(&voter_state_);
+}
+
+void LeaderElection::Run() {
+ VLOG_WITH_PREFIX(1) << "Running leader election.";
+
+ // Initialize voter state tracking.
+ vector<string> other_voter_uuids;
+ voter_state_.clear();
+ for (const RaftPeerPB& peer : config_.peers()) {
+ if (request_.candidate_uuid() == peer.permanent_uuid()) {
DCHECK_EQ(peer.member_type(), RaftPeerPB::VOTER)
<< Substitute("non-voter member $0 tried to start an election; "
"Raft config {$1}",
peer.permanent_uuid(),
- pb_util::SecureShortDebugString(config));
+ pb_util::SecureShortDebugString(config_));
continue;
}
if (peer.member_type() != RaftPeerPB::VOTER) {
continue;
}
- other_voter_uuids_.push_back(peer.permanent_uuid());
+ other_voter_uuids.emplace_back(peer.permanent_uuid());
gscoped_ptr<VoterState> state(new VoterState());
state->peer_uuid = peer.permanent_uuid();
- state->proxy_status = proxy_factory->NewProxy(peer, &state->proxy);
+ state->proxy_status = proxy_factory_->NewProxy(peer, &state->proxy);
InsertOrDie(&voter_state_, peer.permanent_uuid(), state.release());
}
@@ -195,28 +213,18 @@ LeaderElection::LeaderElection(const RaftConfigPB& config,
CHECK_EQ(1, vote_counter_->GetTotalVotesCounted()) << "Candidate must vote for itself first";
// Ensure that existing votes + future votes add up to the expected total.
- CHECK_EQ(vote_counter_->GetTotalVotesCounted() + other_voter_uuids_.size(),
+ CHECK_EQ(vote_counter_->GetTotalVotesCounted() + other_voter_uuids.size(),
vote_counter_->GetTotalExpectedVotes())
<< "Expected different number of voters. Voter UUIDs: ["
- << JoinStringsIterator(other_voter_uuids_.begin(), other_voter_uuids_.end(), ", ")
- << "]; RaftConfig: {" << pb_util::SecureShortDebugString(config) << "}";
-}
-
-LeaderElection::~LeaderElection() {
- std::lock_guard<Lock> guard(lock_);
- DCHECK(has_responded_); // We must always call the callback exactly once.
- STLDeleteValues(&voter_state_);
-}
-
-void LeaderElection::Run() {
- VLOG_WITH_PREFIX(1) << "Running leader election.";
+ << JoinStringsIterator(other_voter_uuids.begin(), other_voter_uuids.end(), ", ")
+ << "]; RaftConfig: {" << pb_util::SecureShortDebugString(config_) << "}";
// Check if we have already won the election (relevant if this is a
// single-node configuration, since we always pre-vote for ourselves).
CheckForDecision();
// The rest of the code below is for a typical multi-node configuration.
- for (const std::string& voter_uuid : other_voter_uuids_) {
+ for (const auto& voter_uuid : other_voter_uuids) {
VoterState* state = nullptr;
{
std::lock_guard<Lock> guard(lock_);
http://git-wip-us.apache.org/repos/asf/kudu/blob/e157663b/src/kudu/consensus/leader_election.h
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/leader_election.h b/src/kudu/consensus/leader_election.h
index f33c268..5e145e2 100644
--- a/src/kudu/consensus/leader_election.h
+++ b/src/kudu/consensus/leader_election.h
@@ -23,10 +23,10 @@
#include <map>
#include <string>
#include <unordered_map>
-#include <vector>
#include "kudu/consensus/consensus.pb.h"
#include "kudu/consensus/consensus_peers.h"
+#include "kudu/consensus/metadata.pb.h"
#include "kudu/consensus/raft_consensus.h"
#include "kudu/gutil/gscoped_ptr.h"
#include "kudu/gutil/macros.h"
@@ -39,8 +39,6 @@
namespace kudu {
namespace consensus {
-class RaftConfigPB;
-
// The vote a peer has given.
enum ElectionVote {
VOTE_DENIED = 0,
@@ -139,14 +137,17 @@ struct ElectionResult {
class LeaderElection : public RefCountedThreadSafe<LeaderElection> {
public:
typedef std::function<void(const ElectionResult&)> ElectionDecisionCallback;
- typedef std::unordered_map<std::string, PeerProxy*> ProxyMap;
// Set up a new leader election driver.
//
+ // 'proxy_factory' must not go out of scope while LeaderElection is alive.
+ //
// The 'vote_counter' must be initialized with the candidate's own yes vote.
- LeaderElection(const RaftConfigPB& config, PeerProxyFactory* proxy_factory,
- const VoteRequestPB& request,
- gscoped_ptr<VoteCounter> vote_counter, MonoDelta timeout,
+ LeaderElection(RaftConfigPB config,
+ PeerProxyFactory* proxy_factory,
+ VoteRequestPB request,
+ gscoped_ptr<VoteCounter> vote_counter,
+ MonoDelta timeout,
ElectionDecisionCallback decision_callback);
// Run the election: send the vote request to followers.
@@ -212,6 +213,12 @@ class LeaderElection : public RefCountedThreadSafe<LeaderElection> {
// Whether we have responded via the callback yet.
bool has_responded_;
+ // Active Raft configuration at election start time.
+ const RaftConfigPB config_;
+
+ // Factory used in the creation of new proxies.
+ PeerProxyFactory* proxy_factory_;
+
// Election request to send to voters.
const VoteRequestPB request_;
@@ -224,10 +231,6 @@ class LeaderElection : public RefCountedThreadSafe<LeaderElection> {
// Callback invoked to notify the caller of an election decision.
const ElectionDecisionCallback decision_callback_;
- // List of all other voters to request votes from.
- // The candidate's own UUID must not be included.
- std::vector<std::string> other_voter_uuids_;
-
// Map of UUID -> VoterState.
VoterStateMap voter_state_;
http://git-wip-us.apache.org/repos/asf/kudu/blob/e157663b/src/kudu/consensus/raft_consensus.cc
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/raft_consensus.cc b/src/kudu/consensus/raft_consensus.cc
index b4ef789..c70abd9 100644
--- a/src/kudu/consensus/raft_consensus.cc
+++ b/src/kudu/consensus/raft_consensus.cc
@@ -507,9 +507,11 @@ Status RaftConsensus::StartElection(ElectionMode mode, ElectionReason reason) {
queue_->GetLastOpIdInLog();
election.reset(new LeaderElection(
- active_config,
+ std::move(active_config),
+ // The RaftConsensus ref passed below ensures that this raw pointer
+ // remains safe to use for the entirety of LeaderElection's life.
peer_proxy_factory_.get(),
- request, std::move(counter), timeout,
+ std::move(request), std::move(counter), timeout,
std::bind(&RaftConsensus::ElectionCallback,
shared_from_this(),
reason, std::placeholders::_1)));