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 2017/12/02 05:30:59 UTC
[1/2] kudu git commit: KUDU-1097. Never evict a leader
Repository: kudu
Updated Branches:
refs/heads/branch-1.6.x 8a570e772 -> d43404166
KUDU-1097. Never evict a leader
The master should never try to evict the leader replica when the tablet
is over-replicated and arbitrary voters are eligible for eviction. This
patch ensures that is the case.
Change-Id: I57adc8c4c4a6a3b5ad0c0b9202407a4dd9e1e8b0
Reviewed-on: http://gerrit.cloudera.org:8080/8680
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-on: http://gerrit.cloudera.org:8080/8736
Tested-by: Mike Percy <mp...@apache.org>
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/44b0bd2a
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/44b0bd2a
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/44b0bd2a
Branch: refs/heads/branch-1.6.x
Commit: 44b0bd2a8f61dca58a6bec3b555a18dffae49bcb
Parents: 8a570e7
Author: Mike Percy <mp...@apache.org>
Authored: Wed Nov 29 02:56:58 2017 -0800
Committer: Mike Percy <mp...@apache.org>
Committed: Sat Dec 2 05:30:32 2017 +0000
----------------------------------------------------------------------
src/kudu/consensus/quorum_util-test.cc | 102 ++++++++++++++++------------
src/kudu/consensus/quorum_util.cc | 13 ++++
src/kudu/consensus/quorum_util.h | 10 +--
src/kudu/master/catalog_manager.cc | 3 +-
4 files changed, 80 insertions(+), 48 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/44b0bd2a/src/kudu/consensus/quorum_util-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/quorum_util-test.cc b/src/kudu/consensus/quorum_util-test.cc
index 461ae7a..cd038fc 100644
--- a/src/kudu/consensus/quorum_util-test.cc
+++ b/src/kudu/consensus/quorum_util-test.cc
@@ -410,16 +410,16 @@ TEST(QuorumUtilTest, CanEvictReplicaVoters) {
AddPeer(&config, "A", V);
AddPeer(&config, "B", V);
AddPeer(&config, "C", V);
- EXPECT_FALSE(CanEvictReplica(config, 3));
- EXPECT_FALSE(CanEvictReplica(config, 2));
+ EXPECT_FALSE(CanEvictReplica(config, "", 3));
+ EXPECT_FALSE(CanEvictReplica(config, "", 2));
}
{
RaftConfigPB config;
AddPeer(&config, "A", V, '?');
AddPeer(&config, "B", V, '?');
AddPeer(&config, "C", V, '-');
- EXPECT_FALSE(CanEvictReplica(config, 3));
- EXPECT_FALSE(CanEvictReplica(config, 2));
+ EXPECT_FALSE(CanEvictReplica(config, "", 3));
+ EXPECT_FALSE(CanEvictReplica(config, "", 2));
}
{
RaftConfigPB config;
@@ -427,18 +427,18 @@ TEST(QuorumUtilTest, CanEvictReplicaVoters) {
AddPeer(&config, "B", V, '-');
AddPeer(&config, "C", V, '+');
string uuid_to_evict;
- ASSERT_TRUE(CanEvictReplica(config, 1, &uuid_to_evict));
+ ASSERT_TRUE(CanEvictReplica(config, "C", 1, &uuid_to_evict));
EXPECT_EQ("B", uuid_to_evict);
- EXPECT_FALSE(CanEvictReplica(config, 3));
- EXPECT_FALSE(CanEvictReplica(config, 2));
+ EXPECT_FALSE(CanEvictReplica(config, "C", 3));
+ EXPECT_FALSE(CanEvictReplica(config, "C", 2));
}
{
RaftConfigPB config;
AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
AddPeer(&config, "B", V);
AddPeer(&config, "C", V);
- EXPECT_FALSE(CanEvictReplica(config, 3));
- EXPECT_FALSE(CanEvictReplica(config, 2));
+ EXPECT_FALSE(CanEvictReplica(config, "A", 3));
+ EXPECT_FALSE(CanEvictReplica(config, "A", 2));
}
{
RaftConfigPB config;
@@ -446,9 +446,9 @@ TEST(QuorumUtilTest, CanEvictReplicaVoters) {
AddPeer(&config, "B", V, '+', {{"REPLACE", false}});
AddPeer(&config, "C", V, '-');
AddPeer(&config, "D", V, '+');
- EXPECT_FALSE(CanEvictReplica(config, 4));
+ EXPECT_FALSE(CanEvictReplica(config, "A", 4));
string uuid_to_evict;
- ASSERT_TRUE(CanEvictReplica(config, 3, &uuid_to_evict));
+ ASSERT_TRUE(CanEvictReplica(config, "A", 3, &uuid_to_evict));
EXPECT_EQ("C", uuid_to_evict);
}
{
@@ -457,9 +457,9 @@ TEST(QuorumUtilTest, CanEvictReplicaVoters) {
AddPeer(&config, "B", V, '?');
AddPeer(&config, "C", V, '+');
AddPeer(&config, "D", V, '+');
- EXPECT_FALSE(CanEvictReplica(config, 4));
+ EXPECT_FALSE(CanEvictReplica(config, "A", 4));
string uuid_to_evict;
- ASSERT_TRUE(CanEvictReplica(config, 3, &uuid_to_evict));
+ ASSERT_TRUE(CanEvictReplica(config, "A", 3, &uuid_to_evict));
EXPECT_EQ("B", uuid_to_evict);
}
{
@@ -468,9 +468,9 @@ TEST(QuorumUtilTest, CanEvictReplicaVoters) {
AddPeer(&config, "B", V, '+');
AddPeer(&config, "C", V, '-');
AddPeer(&config, "D", V, '?', {{"REPLACE", true}});
- EXPECT_FALSE(CanEvictReplica(config, 3));
+ EXPECT_FALSE(CanEvictReplica(config, "A", 3));
string uuid_to_evict;
- ASSERT_TRUE(CanEvictReplica(config, 2, &uuid_to_evict));
+ ASSERT_TRUE(CanEvictReplica(config, "A", 2, &uuid_to_evict));
EXPECT_EQ("D", uuid_to_evict);
}
{
@@ -479,9 +479,9 @@ TEST(QuorumUtilTest, CanEvictReplicaVoters) {
AddPeer(&config, "B", V, '+');
AddPeer(&config, "C", V, '-');
AddPeer(&config, "D", V, '-', {{"REPLACE", true}});
- EXPECT_FALSE(CanEvictReplica(config, 3));
+ EXPECT_FALSE(CanEvictReplica(config, "A", 3));
string uuid_to_evict;
- ASSERT_TRUE(CanEvictReplica(config, 2, &uuid_to_evict));
+ ASSERT_TRUE(CanEvictReplica(config, "A", 2, &uuid_to_evict));
EXPECT_EQ("D", uuid_to_evict);
}
{
@@ -490,9 +490,9 @@ TEST(QuorumUtilTest, CanEvictReplicaVoters) {
AddPeer(&config, "B", V, '+');
AddPeer(&config, "C", V, '-');
AddPeer(&config, "D", V, '+', {{"REPLACE", true}});
- EXPECT_FALSE(CanEvictReplica(config, 3));
+ EXPECT_FALSE(CanEvictReplica(config, "A", 3));
string uuid_to_evict;
- ASSERT_TRUE(CanEvictReplica(config, 2, &uuid_to_evict));
+ ASSERT_TRUE(CanEvictReplica(config, "A", 2, &uuid_to_evict));
EXPECT_EQ("D", uuid_to_evict);
}
{
@@ -501,9 +501,9 @@ TEST(QuorumUtilTest, CanEvictReplicaVoters) {
AddPeer(&config, "B", V, '+');
AddPeer(&config, "C", V, '?');
AddPeer(&config, "D", V, '+', {{"REPLACE", true}});
- EXPECT_FALSE(CanEvictReplica(config, 3));
+ EXPECT_FALSE(CanEvictReplica(config, "A", 3));
string uuid_to_evict;
- ASSERT_TRUE(CanEvictReplica(config, 2, &uuid_to_evict));
+ ASSERT_TRUE(CanEvictReplica(config, "A", 2, &uuid_to_evict));
EXPECT_EQ("D", uuid_to_evict);
}
{
@@ -512,9 +512,9 @@ TEST(QuorumUtilTest, CanEvictReplicaVoters) {
AddPeer(&config, "B", V, '?');
AddPeer(&config, "C", V, '+');
AddPeer(&config, "D", V, '+');
- EXPECT_FALSE(CanEvictReplica(config, 4));
+ EXPECT_FALSE(CanEvictReplica(config, "A", 4));
string uuid_to_evict;
- ASSERT_TRUE(CanEvictReplica(config, 3, &uuid_to_evict));
+ ASSERT_TRUE(CanEvictReplica(config, "A", 3, &uuid_to_evict));
EXPECT_EQ("B", uuid_to_evict);
}
}
@@ -525,20 +525,20 @@ TEST(QuorumUtilTest, CanEvictReplicaNonVoters) {
{
RaftConfigPB config;
AddPeer(&config, "A", V);
- EXPECT_FALSE(CanEvictReplica(config, 1));
+ EXPECT_FALSE(CanEvictReplica(config, "", 1));
}
{
RaftConfigPB config;
AddPeer(&config, "A", V, '+');
- EXPECT_FALSE(CanEvictReplica(config, 1));
+ EXPECT_FALSE(CanEvictReplica(config, "A", 1));
}
{
RaftConfigPB config;
AddPeer(&config, "A", V, '+');
AddPeer(&config, "B", N);
- EXPECT_FALSE(CanEvictReplica(config, 2));
+ EXPECT_FALSE(CanEvictReplica(config, "A", 2));
string uuid_to_evict;
- ASSERT_TRUE(CanEvictReplica(config, 1, &uuid_to_evict));
+ ASSERT_TRUE(CanEvictReplica(config, "A", 1, &uuid_to_evict));
EXPECT_EQ("B", uuid_to_evict);
}
{
@@ -547,18 +547,18 @@ TEST(QuorumUtilTest, CanEvictReplicaNonVoters) {
AddPeer(&config, "B", V, '+');
AddPeer(&config, "C", N, '+');
string uuid_to_evict;
- ASSERT_TRUE(CanEvictReplica(config, 2, &uuid_to_evict));
+ ASSERT_TRUE(CanEvictReplica(config, "A", 2, &uuid_to_evict));
EXPECT_EQ("C", uuid_to_evict);
- ASSERT_TRUE(CanEvictReplica(config, 1, &uuid_to_evict));
+ ASSERT_TRUE(CanEvictReplica(config, "A", 1, &uuid_to_evict));
EXPECT_EQ("C", uuid_to_evict);
}
{
RaftConfigPB config;
AddPeer(&config, "A", V, '+');
AddPeer(&config, "B", N, '-', {{"PROMOTE", true}});
- EXPECT_FALSE(CanEvictReplica(config, 2));
+ EXPECT_FALSE(CanEvictReplica(config, "A", 2));
string uuid_to_evict;
- ASSERT_TRUE(CanEvictReplica(config, 1, &uuid_to_evict));
+ ASSERT_TRUE(CanEvictReplica(config, "A", 1, &uuid_to_evict));
EXPECT_EQ("B", uuid_to_evict);
}
{
@@ -566,9 +566,9 @@ TEST(QuorumUtilTest, CanEvictReplicaNonVoters) {
AddPeer(&config, "A", V, '+');
AddPeer(&config, "B", N, '-');
AddPeer(&config, "C", N);
- EXPECT_FALSE(CanEvictReplica(config, 2));
+ EXPECT_FALSE(CanEvictReplica(config, "A", 2));
string uuid_to_evict;
- ASSERT_TRUE(CanEvictReplica(config, 1, &uuid_to_evict));
+ ASSERT_TRUE(CanEvictReplica(config, "A", 1, &uuid_to_evict));
EXPECT_EQ("B", uuid_to_evict);
}
{
@@ -576,9 +576,9 @@ TEST(QuorumUtilTest, CanEvictReplicaNonVoters) {
AddPeer(&config, "A", V, '+');
AddPeer(&config, "B", N, '?');
AddPeer(&config, "C", N, '+');
- EXPECT_FALSE(CanEvictReplica(config, 2));
+ EXPECT_FALSE(CanEvictReplica(config, "A", 2));
string uuid_to_evict;
- ASSERT_TRUE(CanEvictReplica(config, 1, &uuid_to_evict));
+ ASSERT_TRUE(CanEvictReplica(config, "A", 1, &uuid_to_evict));
EXPECT_EQ("B", uuid_to_evict);
}
{
@@ -586,9 +586,9 @@ TEST(QuorumUtilTest, CanEvictReplicaNonVoters) {
AddPeer(&config, "A", V, '+');
AddPeer(&config, "B", V);
AddPeer(&config, "C", N);
- EXPECT_FALSE(CanEvictReplica(config, 2));
+ EXPECT_FALSE(CanEvictReplica(config, "A", 2));
string uuid_to_evict;
- ASSERT_TRUE(CanEvictReplica(config, 1, &uuid_to_evict));
+ ASSERT_TRUE(CanEvictReplica(config, "A", 1, &uuid_to_evict));
EXPECT_EQ("C", uuid_to_evict);
}
{
@@ -596,17 +596,17 @@ TEST(QuorumUtilTest, CanEvictReplicaNonVoters) {
AddPeer(&config, "A", V, '-');
AddPeer(&config, "B", V);
AddPeer(&config, "C", N);
- EXPECT_FALSE(CanEvictReplica(config, 2));
- EXPECT_FALSE(CanEvictReplica(config, 1));
+ EXPECT_FALSE(CanEvictReplica(config, "", 2));
+ EXPECT_FALSE(CanEvictReplica(config, "", 1));
}
{
RaftConfigPB config;
AddPeer(&config, "A", V, '+');
AddPeer(&config, "B", V, '-');
AddPeer(&config, "C", N, '-', {{"PROMOTE", true}});
- EXPECT_FALSE(CanEvictReplica(config, 2));
+ EXPECT_FALSE(CanEvictReplica(config, "A", 2));
string uuid_to_evict;
- ASSERT_TRUE(CanEvictReplica(config, 1, &uuid_to_evict));
+ ASSERT_TRUE(CanEvictReplica(config, "A", 1, &uuid_to_evict));
}
{
RaftConfigPB config;
@@ -614,12 +614,28 @@ TEST(QuorumUtilTest, CanEvictReplicaNonVoters) {
AddPeer(&config, "B", V, '+');
AddPeer(&config, "C", V, '+');
AddPeer(&config, "D", N, '+', {{"PROMOTE", true}});
- EXPECT_FALSE(CanEvictReplica(config, 3));
+ EXPECT_FALSE(CanEvictReplica(config, "B", 3));
string uuid_to_evict;
- ASSERT_TRUE(CanEvictReplica(config, 2, &uuid_to_evict));
+ ASSERT_TRUE(CanEvictReplica(config, "B", 2, &uuid_to_evict));
EXPECT_EQ("D", uuid_to_evict);
}
}
+// Exhaustively loop through all nodes, each as leader, when over-replicated
+// and ensure that the leader never gets evicted.
+TEST(QuorumUtilTest, TestDontEvictLeader) {
+ const vector<string> nodes = { "A", "B", "C", "D" };
+ RaftConfigPB config;
+ AddPeer(&config, nodes[0], V, '+');
+ AddPeer(&config, nodes[1], V, '+');
+ AddPeer(&config, nodes[2], V, '+');
+ AddPeer(&config, nodes[3], V, '+');
+ for (const auto& leader_node : nodes) {
+ string to_evict;
+ ASSERT_TRUE(CanEvictReplica(config, leader_node, 3, &to_evict));
+ ASSERT_NE(leader_node, to_evict);
+ }
+}
+
} // namespace consensus
} // namespace kudu
http://git-wip-us.apache.org/repos/asf/kudu/blob/44b0bd2a/src/kudu/consensus/quorum_util.cc
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/quorum_util.cc b/src/kudu/consensus/quorum_util.cc
index 8632e3c..c544256 100644
--- a/src/kudu/consensus/quorum_util.cc
+++ b/src/kudu/consensus/quorum_util.cc
@@ -439,8 +439,12 @@ bool IsUnderReplicated(const RaftConfigPB& config, int replication_factor) {
// Whether there is an excess replica to evict.
bool CanEvictReplica(const RaftConfigPB& config,
+ const string& leader_uuid,
int replication_factor,
string* uuid_to_evict) {
+ // If there is no leader, we can't evict anybody.
+ if (leader_uuid.empty()) return false;
+
int num_non_voters_total = 0;
int num_voters_healthy = 0;
@@ -475,6 +479,13 @@ bool CanEvictReplica(const RaftConfigPB& config,
if (healthy && !has_replace) {
++num_voters_healthy;
}
+ // Everything below here is to check for replicas to evict.
+ if (peer_uuid == leader_uuid) {
+ // A leader should always report itself as being healthy.
+ DCHECK(healthy) << peer_uuid << ": " << SecureShortDebugString(config);
+ break;
+ }
+
if (failed && has_replace) {
voter_failed = voter_replace = peer_uuid;
}
@@ -492,6 +503,8 @@ bool CanEvictReplica(const RaftConfigPB& config,
case RaftPeerPB::NON_VOTER:
++num_non_voters_total;
+ DCHECK_NE(peer_uuid, leader_uuid) << peer_uuid
+ << ": non-voter as a leader; " << SecureShortDebugString(config);
if (failed && non_voter_failed.empty()) {
non_voter_failed = peer_uuid;
}
http://git-wip-us.apache.org/repos/asf/kudu/blob/44b0bd2a/src/kudu/consensus/quorum_util.h
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/quorum_util.h b/src/kudu/consensus/quorum_util.h
index b0d12ae..9afe13f 100644
--- a/src/kudu/consensus/quorum_util.h
+++ b/src/kudu/consensus/quorum_util.h
@@ -94,11 +94,13 @@ bool IsUnderReplicated(const RaftConfigPB& config, int replication_factor);
// Check if the given Raft configuration contains at least one extra replica
// which can be removed in accordance with the specified replication
-// factor. If so, then return 'true' and set the UUID of the best suited
-// replica into the 'uuid_to_evict' out parameter. Otherwise, return 'false'.
+// factor and current Raft leader. If so, then return 'true' and set the UUID
+// of the best suited replica into the 'uuid_to_evict' out parameter. Otherwise,
+// return 'false'.
bool CanEvictReplica(const RaftConfigPB& config,
- int replication_factor,
- std::string* uuid_to_evict = nullptr);
+ const std::string& leader_uuid,
+ int replication_factor,
+ std::string* uuid_to_evict = nullptr);
} // namespace consensus
} // namespace kudu
http://git-wip-us.apache.org/repos/asf/kudu/blob/44b0bd2a/src/kudu/master/catalog_manager.cc
----------------------------------------------------------------------
diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc
index 8144d74..abfed16 100644
--- a/src/kudu/master/catalog_manager.cc
+++ b/src/kudu/master/catalog_manager.cc
@@ -3509,7 +3509,8 @@ Status CatalogManager::ProcessTabletReport(
const RaftConfigPB& config = cstate.committed_config();
string to_evict;
if (PREDICT_TRUE(FLAGS_catalog_manager_evict_excess_replicas) &&
- CanEvictReplica(config, replication_factor, &to_evict)) {
+ CanEvictReplica(config, cstate.leader_uuid(), replication_factor,
+ &to_evict)) {
DCHECK(!to_evict.empty());
rpcs.emplace_back(new AsyncEvictReplicaTask(
master_, tablet, cstate, std::move(to_evict)));
[2/2] kudu git commit: KUDU-1097. master: strip health reports from
cstate before persisting
Posted by mp...@apache.org.
KUDU-1097. master: strip health reports from cstate before persisting
The master should not persist configs with health reports in them
because the health reports are inherently transient information.
This patch strips the health reports from the cstate included in each
leader tablet report before persisting them and adds a couple of DCHECKS
in random places to enforce that.
Change-Id: I3e0b076146d888d01676442f6ab3f67cca6bacd6
Reviewed-on: http://gerrit.cloudera.org:8080/8683
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-on: http://gerrit.cloudera.org:8080/8737
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/d4340416
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/d4340416
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/d4340416
Branch: refs/heads/branch-1.6.x
Commit: d43404166c42c295b53aa195d19519bd9261aee7
Parents: 44b0bd2
Author: Mike Percy <mp...@apache.org>
Authored: Wed Nov 29 04:25:12 2017 -0800
Committer: Mike Percy <mp...@apache.org>
Committed: Sat Dec 2 05:30:40 2017 +0000
----------------------------------------------------------------------
src/kudu/master/catalog_manager.cc | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/d4340416/src/kudu/master/catalog_manager.cc
----------------------------------------------------------------------
diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc
index abfed16..acea922 100644
--- a/src/kudu/master/catalog_manager.cc
+++ b/src/kudu/master/catalog_manager.cc
@@ -3461,8 +3461,16 @@ Status CatalogManager::ProcessTabletReport(
tablet_id, ts_desc->ToString(), cstate.committed_config().opid_index(),
cstate.current_term());
+
// 7d(ii). Update the consensus state.
- *tablet->mutable_metadata()->mutable_dirty()->pb.mutable_consensus_state() = cstate;
+ // Strip the health report from the cstate before persisting it.
+ auto* dirty_cstate =
+ tablet->mutable_metadata()->mutable_dirty()->pb.mutable_consensus_state();
+ *dirty_cstate = cstate; // Copy in the updated cstate.
+ // Strip out the health reports from the persisted copy *only*.
+ for (auto& peer : *dirty_cstate->mutable_committed_config()->mutable_peers()) {
+ peer.clear_health_report();
+ }
tablet_was_mutated = true;
// 7d(iii). Delete any replicas from the previous config that are not
@@ -3473,6 +3481,7 @@ Status CatalogManager::ProcessTabletReport(
InsertOrDie(¤t_member_uuids, p.permanent_uuid());
}
for (const auto& p : prev_cstate.committed_config().peers()) {
+ DCHECK(!p.has_health_report()); // Health report shouldn't be persisted.
const string& peer_uuid = p.permanent_uuid();
if (!ContainsKey(current_member_uuids, peer_uuid)) {
rpcs.emplace_back(new AsyncDeleteReplica(
@@ -4044,6 +4053,7 @@ Status CatalogManager::BuildLocationsForTablet(
const ConsensusStatePB& cstate = l_tablet.data().pb.consensus_state();
for (const consensus::RaftPeerPB& peer : cstate.committed_config().peers()) {
+ DCHECK(!peer.has_health_report()); // Health report shouldn't be persisted.
// TODO(adar): GetConsensusRole() iterates over all of the peers, making this an
// O(n^2) loop. If replication counts get high, it should be optimized.
switch (filter) {