You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by al...@apache.org on 2017/12/12 19:21:26 UTC
kudu git commit: KUDU-1097: more robust criteria for replica eviction
Repository: kudu
Updated Branches:
refs/heads/master ff535b521 -> 2a6de8ea4
KUDU-1097: more robust criteria for replica eviction
Updated the replica eviction criteria to address scenarios where newly
added replicas fail (before or after promotion) one after another
prior to eviction of the original failed voter replica. With this
patch, the replacement process should not end up with placing failed
replicas at every available tablet server, deadlocking the replacement
process. Added corresponding unit tests to cover the updated behavior
of the consensus::CanEvictReplica() utility function.
Updated the replica eviction criteria for more robust handling of the
'replace' attribute. In this context, the case when the leader replica
itself is marked with the 'replace' attribute is now handled properly.
Added unit tests to cover appropriate cases.
Also, enabled the AdminCliTest.TestMoveTablet_KUDU_1097 test scenario
since now it should not be flaky anymore.
As an additional touch, updated the FailedTabletCopy scenario of the
RaftConsensusNonVoterITest to provide more comprehensive coverage.
Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
Reviewed-on: http://gerrit.cloudera.org:8080/8679
Tested-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/2a6de8ea
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/2a6de8ea
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/2a6de8ea
Branch: refs/heads/master
Commit: 2a6de8ea46ae699bffb0435119fc62a15200eab7
Parents: ff535b5
Author: Alexey Serbin <as...@cloudera.com>
Authored: Wed Nov 29 02:15:06 2017 -0800
Committer: Alexey Serbin <as...@cloudera.com>
Committed: Tue Dec 12 19:20:25 2017 +0000
----------------------------------------------------------------------
src/kudu/consensus/quorum_util-test.cc | 682 +++++++++++++++++--
src/kudu/consensus/quorum_util.cc | 183 ++++-
.../raft_consensus_nonvoter-itest.cc | 66 +-
src/kudu/master/catalog_manager.cc | 10 +-
src/kudu/tools/kudu-admin-test.cc | 2 +-
5 files changed, 847 insertions(+), 96 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/2a6de8ea/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 cd038fc..3f525c0 100644
--- a/src/kudu/consensus/quorum_util-test.cc
+++ b/src/kudu/consensus/quorum_util-test.cc
@@ -26,23 +26,46 @@
#include "kudu/common/common.pb.h"
#include "kudu/consensus/metadata.pb.h"
#include "kudu/consensus/quorum_util.h"
+#include "kudu/gutil/strings/substitute.h"
#include "kudu/util/status.h"
#include "kudu/util/test_macros.h"
using std::string;
using std::unique_ptr;
using std::vector;
+using strings::Substitute;
namespace kudu {
namespace consensus {
// Handy notation of membership types used by AddPeer(), etc.
-constexpr auto N = RaftPeerPB::NON_VOTER;
-constexpr auto U = RaftPeerPB::UNKNOWN_MEMBER_TYPE;
-constexpr auto V = RaftPeerPB::VOTER;
+constexpr auto N = RaftPeerPB::NON_VOTER; // NOLINT(readability-identifier-naming)
+constexpr auto U = RaftPeerPB::UNKNOWN_MEMBER_TYPE; // NOLINT(readability-identifier-naming)
+constexpr auto V = RaftPeerPB::VOTER; // NOLINT(readability-identifier-naming)
+
+// The various possible health statuses.
+constexpr auto kHealthStatuses = { '?', '-', '+' };
typedef std::pair<string, bool> Attr;
+static void SetOverallHealth(HealthReportPB* health_report,
+ char overall_health) {
+ switch (overall_health) {
+ case '+':
+ health_report->set_overall_health(HealthReportPB::HEALTHY);
+ break;
+ case '-':
+ health_report->set_overall_health(HealthReportPB::FAILED);
+ break;
+ case '?':
+ health_report->set_overall_health(HealthReportPB::UNKNOWN);
+ break;
+ default:
+ FAIL() << overall_health << ": unexpected replica health status";
+ break;
+ }
+}
+
// Add a consensus peer into the specified configuration.
static void AddPeer(RaftConfigPB* config,
const string& uuid,
@@ -55,20 +78,7 @@ static void AddPeer(RaftConfigPB* config,
peer->set_member_type(type);
if (overall_health) {
unique_ptr<HealthReportPB> health_report(new HealthReportPB);
- switch (*overall_health) {
- case '+':
- health_report->set_overall_health(HealthReportPB::HEALTHY);
- break;
- case '-':
- health_report->set_overall_health(HealthReportPB::FAILED);
- break;
- case '?':
- health_report->set_overall_health(HealthReportPB::UNKNOWN);
- break;
- default:
- FAIL() << *overall_health << ": unexpected replica health status";
- break;
- }
+ SetOverallHealth(health_report.get(), *overall_health);
peer->set_allocated_health_report(health_report.release());
}
if (!attrs.empty()) {
@@ -86,6 +96,32 @@ static void AddPeer(RaftConfigPB* config,
}
}
+static void PromotePeer(RaftConfigPB* config, const string& peer_uuid) {
+ RaftPeerPB* peer_pb;
+ const Status s = GetRaftConfigMember(config, peer_uuid, &peer_pb);
+ if (!s.ok()) {
+ FAIL() << peer_uuid << ": " << s.ToString();
+ }
+ peer_pb->set_member_type(V);
+ //peer_pb->mutable_attrs()->clear_promote();
+ peer_pb->mutable_attrs()->set_promote(false);
+}
+
+static void RemovePeer(RaftConfigPB* config, const string& peer_uuid) {
+ if (!RemoveFromRaftConfig(config, peer_uuid)) {
+ FAIL() << peer_uuid << ": peer is not in the config";
+ }
+}
+
+static void SetPeerHealth(RaftConfigPB* config, const string& uuid, char health) {
+ RaftPeerPB* peer_pb;
+ const Status s = GetRaftConfigMember(config, uuid, &peer_pb);
+ if (!s.ok()) {
+ FAIL() << "unexpected failure from GetRaftConfigMember(): " << s.ToString();
+ }
+ SetOverallHealth(peer_pb->mutable_health_report(), health);
+}
+
TEST(QuorumUtilTest, TestMemberExtraction) {
RaftConfigPB config;
AddPeer(&config, "A", V);
@@ -359,7 +395,8 @@ TEST(QuorumUtilTest, IsUnderReplicated) {
AddPeer(&config, "C", V, '-');
AddPeer(&config, "D", N, '+');
EXPECT_FALSE(IsUnderReplicated(config, 2));
- // The replica does not have the PROMOTE attribute, so a new one is needed.
+ // The non-voter replica does not have the PROMOTE attribute,
+ // so a new one is needed.
EXPECT_TRUE(IsUnderReplicated(config, 3));
EXPECT_TRUE(IsUnderReplicated(config, 4));
}
@@ -400,6 +437,16 @@ TEST(QuorumUtilTest, IsUnderReplicated) {
AddPeer(&config, "E", N, '+', {{"PROMOTE", false}});
EXPECT_TRUE(IsUnderReplicated(config, 3));
}
+ {
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+');
+ AddPeer(&config, "B", V, '-');
+ AddPeer(&config, "C", V, '-');
+ // The catalog manager will be able to carry on the required update of the
+ // configuration after achieving the majority.
+ // TODO(aserbin): add an integration test for that.
+ EXPECT_TRUE(IsUnderReplicated(config, 3));
+ }
}
// Verify logic of the kudu::consensus::CanEvictReplica(), anticipating
@@ -423,14 +470,26 @@ TEST(QuorumUtilTest, CanEvictReplicaVoters) {
}
{
RaftConfigPB config;
+ AddPeer(&config, "A", V, '+');
+ AddPeer(&config, "B", V, '+');
+ AddPeer(&config, "C", V, '-');
+ string to_evict;
+ ASSERT_TRUE(CanEvictReplica(config, "A", 1, &to_evict));
+ EXPECT_EQ("C", to_evict);
+ ASSERT_TRUE(CanEvictReplica(config, "A", 2));
+ EXPECT_EQ("C", to_evict);
+ EXPECT_FALSE(CanEvictReplica(config, "A", 3));
+ }
+ {
+ RaftConfigPB config;
AddPeer(&config, "A", V, '?');
AddPeer(&config, "B", V, '-');
AddPeer(&config, "C", V, '+');
- string uuid_to_evict;
- ASSERT_TRUE(CanEvictReplica(config, "C", 1, &uuid_to_evict));
- EXPECT_EQ("B", uuid_to_evict);
- EXPECT_FALSE(CanEvictReplica(config, "C", 3));
+ // Not safe to evict because we don't have enough healthy nodes to commit
+ // the eviction.
+ EXPECT_FALSE(CanEvictReplica(config, "C", 1));
EXPECT_FALSE(CanEvictReplica(config, "C", 2));
+ EXPECT_FALSE(CanEvictReplica(config, "C", 3));
}
{
RaftConfigPB config;
@@ -462,38 +521,35 @@ TEST(QuorumUtilTest, CanEvictReplicaVoters) {
ASSERT_TRUE(CanEvictReplica(config, "A", 3, &uuid_to_evict));
EXPECT_EQ("B", uuid_to_evict);
}
- {
+ for (char health_status : kHealthStatuses) {
+ SCOPED_TRACE(Substitute("replica health status '$0'", health_status));
RaftConfigPB config;
AddPeer(&config, "A", V, '+');
AddPeer(&config, "B", V, '+');
AddPeer(&config, "C", V, '-');
- AddPeer(&config, "D", V, '?', {{"REPLACE", true}});
- EXPECT_FALSE(CanEvictReplica(config, "A", 3));
+ AddPeer(&config, "D", V, health_status, {{"REPLACE", true}});
+ // For replication factors <= 3 we will be able to commit the eviction of D
+ // with only A and B, regardless of D's health and regardless of the
+ // desired replication factor.
string uuid_to_evict;
ASSERT_TRUE(CanEvictReplica(config, "A", 2, &uuid_to_evict));
- EXPECT_EQ("D", uuid_to_evict);
- }
- {
- RaftConfigPB config;
- AddPeer(&config, "A", V, '+');
- AddPeer(&config, "B", V, '+');
- AddPeer(&config, "C", V, '-');
- AddPeer(&config, "D", V, '-', {{"REPLACE", true}});
- EXPECT_FALSE(CanEvictReplica(config, "A", 3));
- string uuid_to_evict;
- ASSERT_TRUE(CanEvictReplica(config, "A", 2, &uuid_to_evict));
- EXPECT_EQ("D", uuid_to_evict);
- }
- {
- RaftConfigPB config;
- AddPeer(&config, "A", V, '+');
- AddPeer(&config, "B", V, '+');
- AddPeer(&config, "C", V, '-');
- AddPeer(&config, "D", V, '+', {{"REPLACE", true}});
- EXPECT_FALSE(CanEvictReplica(config, "A", 3));
- string uuid_to_evict;
- ASSERT_TRUE(CanEvictReplica(config, "A", 2, &uuid_to_evict));
- EXPECT_EQ("D", uuid_to_evict);
+ // The priority of voter replica replacement (decreasing):
+ // * failed & slated for replacement
+ // * failed
+ // * ...
+ if (health_status == '-') {
+ EXPECT_EQ("D", uuid_to_evict);
+ } else {
+ EXPECT_EQ("C", uuid_to_evict);
+ }
+ ASSERT_TRUE(CanEvictReplica(config, "A", 3, &uuid_to_evict));
+ if (health_status == '-') {
+ EXPECT_EQ("D", uuid_to_evict);
+ } else {
+ EXPECT_EQ("C", uuid_to_evict);
+ }
+ // Since we are not over-replicated, we will not evict in this case.
+ EXPECT_FALSE(CanEvictReplica(config, "A", 4));
}
{
RaftConfigPB config;
@@ -501,7 +557,15 @@ TEST(QuorumUtilTest, CanEvictReplicaVoters) {
AddPeer(&config, "B", V, '+');
AddPeer(&config, "C", V, '?');
AddPeer(&config, "D", V, '+', {{"REPLACE", true}});
+
+ // For the replication factor 3, it's too early to evict 'C': it might be
+ // in a good health, actually (reported, say, next heartbeat). Evicting 'D'
+ // at this step is not a good idea neither: the 'C' might appear to fail,
+ // and then it's better to keep 'D' around to provide the required
+ // replication factor. It's necessary to wait for more deterministic status
+ // of replica 'C' before making proper eviction decision.
EXPECT_FALSE(CanEvictReplica(config, "A", 3));
+
string uuid_to_evict;
ASSERT_TRUE(CanEvictReplica(config, "A", 2, &uuid_to_evict));
EXPECT_EQ("D", uuid_to_evict);
@@ -556,8 +620,11 @@ TEST(QuorumUtilTest, CanEvictReplicaNonVoters) {
RaftConfigPB config;
AddPeer(&config, "A", V, '+');
AddPeer(&config, "B", N, '-', {{"PROMOTE", true}});
- EXPECT_FALSE(CanEvictReplica(config, "A", 2));
string uuid_to_evict;
+ // It's always safe to evict an unhealthy non-voter if we have enough
+ // healthy voters to commit the config change.
+ ASSERT_TRUE(CanEvictReplica(config, "A", 2, &uuid_to_evict));
+ EXPECT_EQ("B", uuid_to_evict);
ASSERT_TRUE(CanEvictReplica(config, "A", 1, &uuid_to_evict));
EXPECT_EQ("B", uuid_to_evict);
}
@@ -566,8 +633,9 @@ TEST(QuorumUtilTest, CanEvictReplicaNonVoters) {
AddPeer(&config, "A", V, '+');
AddPeer(&config, "B", N, '-');
AddPeer(&config, "C", N);
- EXPECT_FALSE(CanEvictReplica(config, "A", 2));
string uuid_to_evict;
+ ASSERT_TRUE(CanEvictReplica(config, "A", 2, &uuid_to_evict));
+ EXPECT_EQ("B", uuid_to_evict);
ASSERT_TRUE(CanEvictReplica(config, "A", 1, &uuid_to_evict));
EXPECT_EQ("B", uuid_to_evict);
}
@@ -584,11 +652,32 @@ TEST(QuorumUtilTest, CanEvictReplicaNonVoters) {
{
RaftConfigPB config;
AddPeer(&config, "A", V, '+');
+ AddPeer(&config, "B", V, '+');
+ AddPeer(&config, "C", N);
+ string uuid_to_evict;
+ ASSERT_TRUE(CanEvictReplica(config, "A", 2, &uuid_to_evict));
+ EXPECT_EQ("C", 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", V);
AddPeer(&config, "C", N);
EXPECT_FALSE(CanEvictReplica(config, "A", 2));
+ // Would evict a non-voter first, but it's not known whether the majority
+ // of the voter replicas are on-line to commence the operation: that's
+ // because the state of B is unknown. So, in this case the voter replica B
+ // will be removed first.
string uuid_to_evict;
ASSERT_TRUE(CanEvictReplica(config, "A", 1, &uuid_to_evict));
+ EXPECT_EQ("B", uuid_to_evict);
+
+ RemovePeer(&config, "B");
+ // Now, having just a single online replica, it's possible to evict the
+ // failed non-voter replica C.
+ ASSERT_TRUE(CanEvictReplica(config, "A", 1, &uuid_to_evict));
EXPECT_EQ("C", uuid_to_evict);
}
{
@@ -605,8 +694,18 @@ TEST(QuorumUtilTest, CanEvictReplicaNonVoters) {
AddPeer(&config, "B", V, '-');
AddPeer(&config, "C", N, '-', {{"PROMOTE", true}});
EXPECT_FALSE(CanEvictReplica(config, "A", 2));
+ // Would evict a non-voter first, but replica B is reported as failed and
+ // the configuration does not have enough healthy voter replicas to have a
+ // majority of votes. So, the voter replica B will be removed first.
string uuid_to_evict;
ASSERT_TRUE(CanEvictReplica(config, "A", 1, &uuid_to_evict));
+ EXPECT_EQ("B", uuid_to_evict);
+
+ RemovePeer(&config, "B");
+ // Now, having just a single online replica, it's possible to evict the
+ // failed non-voter replica C.
+ ASSERT_TRUE(CanEvictReplica(config, "A", 1, &uuid_to_evict));
+ EXPECT_EQ("C", uuid_to_evict);
}
{
RaftConfigPB config;
@@ -619,6 +718,29 @@ TEST(QuorumUtilTest, CanEvictReplicaNonVoters) {
ASSERT_TRUE(CanEvictReplica(config, "B", 2, &uuid_to_evict));
EXPECT_EQ("D", uuid_to_evict);
}
+ {
+ // Make sure failed non-voter replicas are removed from the configuration to
+ // avoid polluting all tablet servers with failed non-voter replicas.
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '-');
+ AddPeer(&config, "B", V, '+');
+ AddPeer(&config, "C", V, '+');
+ AddPeer(&config, "D", N, '-', {{"PROMOTE", true}});
+ string uuid_to_evict;
+ ASSERT_TRUE(CanEvictReplica(config, "B", 4, &uuid_to_evict));
+ EXPECT_EQ("D", uuid_to_evict);
+ ASSERT_TRUE(CanEvictReplica(config, "C", 3, &uuid_to_evict));
+ EXPECT_EQ("D", uuid_to_evict);
+ }
+ {
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '-');
+ AddPeer(&config, "B", V, '+');
+ AddPeer(&config, "C", V, '+');
+ AddPeer(&config, "D", N, '?', {{"PROMOTE", true}});
+ EXPECT_FALSE(CanEvictReplica(config, "B", 3));
+ EXPECT_FALSE(CanEvictReplica(config, "B", 4));
+ }
}
// Exhaustively loop through all nodes, each as leader, when over-replicated
@@ -637,5 +759,465 @@ TEST(QuorumUtilTest, TestDontEvictLeader) {
}
}
+// This is a testcase for tablet configurations which can simultaneously be
+// under-replicated and contain a replica suitable for eviction.
+TEST(QuorumUtilTest, TooManyVoters) {
+ {
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+');
+ AddPeer(&config, "B", V, '+');
+ AddPeer(&config, "C", V, '?');
+ AddPeer(&config, "D", V, '-');
+ string to_evict;
+ ASSERT_TRUE(CanEvictReplica(config, "A", 3, &to_evict));
+ EXPECT_EQ("D", to_evict);
+ EXPECT_FALSE(IsUnderReplicated(config, 3));
+ }
+ {
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+');
+ AddPeer(&config, "B", V, '+');
+ AddPeer(&config, "C", V, '-');
+ AddPeer(&config, "D", V, '-');
+ string to_evict;
+ ASSERT_TRUE(CanEvictReplica(config, "A", 3, &to_evict));
+ EXPECT_TRUE(to_evict == "C" || to_evict == "D") << to_evict;
+ EXPECT_TRUE(IsUnderReplicated(config, 3));
+ }
+}
+
+// Basic scenarios involving replicas with the REPLACE attribute set.
+TEST(QuorumUtilTest, ReplaceAttributeBasic) {
+ {
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+');
+ AddPeer(&config, "C", V, '+');
+ EXPECT_FALSE(CanEvictReplica(config, "A", 3));
+ EXPECT_TRUE(IsUnderReplicated(config, 3));
+ }
+ {
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+');
+ AddPeer(&config, "C", V, '+');
+ AddPeer(&config, "D", V, '+');
+ EXPECT_FALSE(CanEvictReplica(config, "A", 3));
+ EXPECT_FALSE(IsUnderReplicated(config, 3));
+ }
+ for (auto health_status : { '-', '?' }) {
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, health_status, {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+');
+ AddPeer(&config, "C", V, '+');
+ AddPeer(&config, "D", V, '+');
+ for (const auto& leader_replica : { "B", "C", "D" }) {
+ SCOPED_TRACE(Substitute("health status '$0', leader $1",
+ health_status, leader_replica));
+ string to_evict;
+ ASSERT_TRUE(CanEvictReplica(config, leader_replica, 3, &to_evict));
+ EXPECT_EQ("A", to_evict);
+ EXPECT_FALSE(IsUnderReplicated(config, 3));
+ }
+ }
+ {
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+');
+ AddPeer(&config, "C", V, '+');
+ AddPeer(&config, "D", V, '+');
+ AddPeer(&config, "E", V, '+');
+ string to_evict;
+ ASSERT_TRUE(CanEvictReplica(config, "A", 3, &to_evict));
+ EXPECT_NE("A", to_evict);
+ EXPECT_FALSE(IsUnderReplicated(config, 3));
+
+ for (const auto& leader_replica : { "B", "C", "D", "E" }) {
+ string to_evict;
+ ASSERT_TRUE(CanEvictReplica(config, leader_replica, 3, &to_evict));
+ EXPECT_EQ("A", to_evict);
+ }
+ }
+ for (auto replica_health : kHealthStatuses) {
+ SCOPED_TRACE(Substitute("replica health status '$0'", replica_health));
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "C", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "D", V, replica_health, {{"REPLACE", true}});
+ string to_evict;
+ ASSERT_TRUE(CanEvictReplica(config, "A", 3, &to_evict));
+ if (replica_health == '+') {
+ EXPECT_NE("A", to_evict);
+ } else {
+ EXPECT_EQ("D", to_evict);
+ }
+ EXPECT_TRUE(IsUnderReplicated(config, 3));
+ }
+ for (auto health_status : { '?', '-' }) {
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, health_status, {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "C", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "D", V, '+');
+ for (const auto& leader_replica : { "B", "C", "D" }) {
+ SCOPED_TRACE(Substitute("health status '$0', leader $1",
+ health_status, leader_replica));
+ string to_evict;
+ ASSERT_TRUE(CanEvictReplica(config, leader_replica, 3, &to_evict));
+ EXPECT_EQ("A", to_evict);
+ EXPECT_TRUE(IsUnderReplicated(config, 3));
+ }
+ }
+ {
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "C", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "D", V, '-');
+ string to_evict;
+ ASSERT_TRUE(CanEvictReplica(config, "A", 3, &to_evict));
+ EXPECT_EQ("D", to_evict);
+ EXPECT_TRUE(IsUnderReplicated(config, 3));
+ }
+ {
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "C", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "D", V, '?');
+ EXPECT_FALSE(CanEvictReplica(config, "B", 3));
+ EXPECT_TRUE(IsUnderReplicated(config, 3));
+ }
+ for (auto health_status : { '?', '-' }) {
+ SCOPED_TRACE(Substitute("health status '$0'", health_status));
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "C", V, '?');
+ AddPeer(&config, "D", V, health_status, {{"REPLACE", true}});
+ AddPeer(&config, "E", V, '+');
+ string to_evict;
+ ASSERT_TRUE(CanEvictReplica(config, "A", 3, &to_evict));
+ EXPECT_EQ("D", to_evict);
+ EXPECT_TRUE(IsUnderReplicated(config, 3));
+ }
+}
+
+// Test specific to the scenarios where the leader replica itself marked with
+// the 'REPLACE' attribute.
+TEST(QuorumUtilTest, LeaderReplicaWithReplaceAttribute) {
+ // Healthy excess voter replicas (both voters and non-voters) should not be
+ // evicted when the leader is marked with the 'REPLACE' attribute.
+ for (auto health_status : { '+', '?' }) {
+ SCOPED_TRACE(Substitute("non-voter replica with status '$0'", health_status));
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+');
+ AddPeer(&config, "C", V, '+');
+ AddPeer(&config, "D", N, health_status, {{"PROMOTE", true}});
+ EXPECT_FALSE(CanEvictReplica(config, "A", 3));
+ EXPECT_FALSE(IsUnderReplicated(config, 3));
+ }
+ for (auto health_status : { '+', '?' }) {
+ SCOPED_TRACE(Substitute("voter replica with status '$0'", health_status));
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+');
+ AddPeer(&config, "C", V, '+');
+ AddPeer(&config, "D", V, health_status);
+ EXPECT_FALSE(CanEvictReplica(config, "A", 3));
+ EXPECT_FALSE(IsUnderReplicated(config, 3));
+ }
+ for (auto promote : { false, true }) {
+ SCOPED_TRACE(Substitute("failed non-voter replica with PROMOTE attribute $0",
+ promote ? "set" : "unset"));
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+');
+ AddPeer(&config, "C", V, '+');
+ AddPeer(&config, "D", N, '-', {{"PROMOTE", promote}});
+ string to_evict;
+ ASSERT_TRUE(CanEvictReplica(config, "A", 3, &to_evict));
+ EXPECT_EQ("D", to_evict);
+ EXPECT_TRUE(IsUnderReplicated(config, 3));
+ }
+ {
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+');
+ AddPeer(&config, "C", V, '+');
+ AddPeer(&config, "D", V, '-');
+ string to_evict;
+ ASSERT_TRUE(CanEvictReplica(config, "A", 3, &to_evict));
+ EXPECT_EQ("D", to_evict);
+ EXPECT_TRUE(IsUnderReplicated(config, 3));
+ }
+ {
+ // Current algorithm is conservative in the cases like below, but we might
+ // evict non-voter replica 'E' which does not have the PROMOTE attribute.
+ // TODO(aserbin): clarify on this.
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+');
+ AddPeer(&config, "C", V, '+');
+ AddPeer(&config, "D", N, '+', {{"PROMOTE", true}});
+ AddPeer(&config, "E", N, '+');
+ EXPECT_FALSE(CanEvictReplica(config, "A", 3));
+ EXPECT_FALSE(IsUnderReplicated(config, 3));
+ }
+ {
+ // The non-voter replica does not have the 'promote' attribute, so
+ // it should be evicted since it's not going to become a voter anyway,
+ // and we don't support standby non-voter replicas at this point.
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+');
+ AddPeer(&config, "C", V, '+');
+ AddPeer(&config, "D", V, '+');
+ AddPeer(&config, "E", N, '+');
+ string to_evict;
+ ASSERT_TRUE(CanEvictReplica(config, "A", 3, &to_evict));
+ EXPECT_EQ("E", to_evict);
+ EXPECT_FALSE(IsUnderReplicated(config, 3));
+ }
+ {
+ // In the case below the non-voter replica 'D' is not needed. The
+ // configuration like that might be the result of an attempt to replace 'D'
+ // which was previously reported as failed. However, by the time the newly
+ // added replica caught up with the leader, replica 'E' was back on-line.
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+');
+ AddPeer(&config, "C", V, '+');
+ AddPeer(&config, "D", N, '+', {{"PROMOTE", true}});
+ AddPeer(&config, "E", V, '+');
+ string to_evict;
+ ASSERT_TRUE(CanEvictReplica(config, "A", 3, &to_evict));
+ EXPECT_EQ("D", to_evict);
+ EXPECT_FALSE(IsUnderReplicated(config, 3));
+ }
+}
+
+// This test is specific for various scenarios when multiple replicas have the
+// REPLACE attribute set.
+TEST(QuorumUtilTest, MultipleReplicasWithReplaceAttribute) {
+ for (auto replica_type : { N, V }) {
+ SCOPED_TRACE(Substitute("replica of $0 type",
+ RaftPeerPB::MemberType_Name(replica_type)));
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "C", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "D", replica_type, '-');
+ string to_evict;
+ ASSERT_TRUE(CanEvictReplica(config, "A", 3, &to_evict));
+ EXPECT_EQ("D", to_evict);
+ EXPECT_TRUE(IsUnderReplicated(config, 3));
+ }
+ for (auto replica_health : { '+', '?' }) {
+ SCOPED_TRACE(Substitute("NON_VOTER replica with health status '$0'", replica_health));
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "C", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "D", N, replica_health);
+ EXPECT_FALSE(CanEvictReplica(config, "A", 3));
+ EXPECT_TRUE(IsUnderReplicated(config, 3));
+ }
+ for (const auto& leader_replica : { "A", "B", "C" }) {
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "C", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "D", V, '?');
+ EXPECT_FALSE(CanEvictReplica(config, leader_replica, 3));
+ EXPECT_TRUE(IsUnderReplicated(config, 3));
+ }
+ for (const auto& leader_replica : { "A", "C" }) {
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "C", V, '+');
+ AddPeer(&config, "D", N, '+', {{"PROMOTE", true}});
+ AddPeer(&config, "E", N, '+', {{"PROMOTE", true}});
+ EXPECT_FALSE(CanEvictReplica(config, leader_replica, 3));
+ EXPECT_FALSE(IsUnderReplicated(config, 3));
+ }
+ {
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "C", V, '+', {{"REPLACE", true}});
+ EXPECT_FALSE(CanEvictReplica(config, "A", 3));
+ EXPECT_TRUE(IsUnderReplicated(config, 3));
+ }
+ {
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "C", V, '+');
+ AddPeer(&config, "D", V, '+');
+ string to_evict;
+ ASSERT_TRUE(CanEvictReplica(config, "A", 3, &to_evict));
+ EXPECT_EQ("B", to_evict);
+ EXPECT_TRUE(IsUnderReplicated(config, 3));
+ }
+ {
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "C", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "D", V, '+');
+ string to_evict;
+ ASSERT_TRUE(CanEvictReplica(config, "A", 3, &to_evict));
+ EXPECT_TRUE(to_evict == "B" || to_evict == "C");
+ EXPECT_TRUE(IsUnderReplicated(config, 3));
+ }
+ {
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+');
+ AddPeer(&config, "C", V, '+');
+ AddPeer(&config, "D", N, '+');
+ EXPECT_FALSE(CanEvictReplica(config, "A", 3));
+ // The non-voter replica does not have the PROMOTE attribute, so it the
+ // configuration should be considered under-replicated.
+ EXPECT_TRUE(IsUnderReplicated(config, 3));
+ }
+ for (auto replica_status : { '+', '?' }) {
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+');
+ AddPeer(&config, "C", V, '+');
+ AddPeer(&config, "D", N, replica_status, {{"PROMOTE", true}});
+ EXPECT_FALSE(CanEvictReplica(config, "A", 3));
+ EXPECT_FALSE(IsUnderReplicated(config, 3));
+ }
+ {
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+');
+ AddPeer(&config, "C", V, '+');
+ AddPeer(&config, "D", N, '-');
+ string to_evict;
+ ASSERT_TRUE(CanEvictReplica(config, "A", 3, &to_evict));
+ EXPECT_EQ("D", to_evict);
+ EXPECT_TRUE(IsUnderReplicated(config, 3));
+ }
+ {
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "C", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "D", V, '+');
+ AddPeer(&config, "E", V, '+');
+ string to_evict;
+ ASSERT_TRUE(CanEvictReplica(config, "A", 3, &to_evict));
+ EXPECT_TRUE(to_evict == "B" || to_evict == "C");
+ EXPECT_TRUE(IsUnderReplicated(config, 3));
+ }
+ {
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "C", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "D", V, '+');
+ AddPeer(&config, "E", V, '+');
+ AddPeer(&config, "F", V, '+');
+
+ for (const auto& leader_replica : { "A", "B", "C" }) {
+ string to_evict;
+ ASSERT_TRUE(CanEvictReplica(config, leader_replica, 3, &to_evict));
+ EXPECT_TRUE(to_evict == "A" || to_evict == "B" || to_evict == "C");
+ EXPECT_NE(leader_replica, to_evict);
+ }
+ for (const auto& leader_replica : { "D", "E", "F" }) {
+ string to_evict;
+ ASSERT_TRUE(CanEvictReplica(config, leader_replica, 3, &to_evict));
+ EXPECT_TRUE(to_evict == "A" || to_evict == "B" || to_evict == "C");
+ }
+ EXPECT_FALSE(IsUnderReplicated(config, 3));
+ }
+}
+
+// A scenario of replica replacement where the replica added for replacement
+// of a failed one also fails. The system should end up replacing both failed
+// replicas.
+TEST(QuorumUtilTest, NewlyPromotedReplicaCrashes) {
+ constexpr auto kReplicationFactor = 3;
+
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+');
+ AddPeer(&config, "B", V, '+');
+ AddPeer(&config, "C", V, '+');
+
+ EXPECT_FALSE(CanEvictReplica(config, "A", kReplicationFactor));
+ EXPECT_FALSE(IsUnderReplicated(config, kReplicationFactor));
+
+ // Replica B fails.
+ SetPeerHealth(&config, "B", '-');
+ EXPECT_FALSE(CanEvictReplica(config, "A", kReplicationFactor));
+ EXPECT_TRUE(IsUnderReplicated(config, kReplicationFactor));
+
+ // Adding a non-voter to replace B.
+ AddPeer(&config, "D", N, '?', {{"PROMOTE", true}});
+ EXPECT_FALSE(CanEvictReplica(config, "A", kReplicationFactor));
+ EXPECT_FALSE(IsUnderReplicated(config, kReplicationFactor));
+
+ // The new non-voter replica becomes healthy.
+ SetPeerHealth(&config, "D", '+');
+ EXPECT_FALSE(CanEvictReplica(config, "A", kReplicationFactor));
+ EXPECT_FALSE(IsUnderReplicated(config, kReplicationFactor));
+
+ // The newly added non-voter replica is promoted.
+ PromotePeer(&config, "D");
+ {
+ // B would be evicted, if it's reported as is.
+ string to_evict;
+ ASSERT_TRUE(CanEvictReplica(config, "A", kReplicationFactor, &to_evict));
+ EXPECT_EQ("B", to_evict);
+ }
+ EXPECT_FALSE(IsUnderReplicated(config, kReplicationFactor));
+
+ // However, the newly promoted replica crashes prior to B getting evicted.
+ // The system should add a new replica for replacement.
+
+ // We cannot evict because we don't have enough healthy voters to commit
+ // the eviction config change.
+ SetPeerHealth(&config, "D", '?');
+ string to_evict;
+ ASSERT_TRUE(CanEvictReplica(config, "A", kReplicationFactor, &to_evict));
+ EXPECT_EQ("B", to_evict);
+ EXPECT_FALSE(IsUnderReplicated(config, kReplicationFactor));
+
+ SetPeerHealth(&config, "D", '-');
+ ASSERT_TRUE(CanEvictReplica(config, "A", kReplicationFactor, &to_evict));
+ EXPECT_TRUE(to_evict == "B" || to_evict == "D") << to_evict;
+ EXPECT_TRUE(IsUnderReplicated(config, kReplicationFactor));
+
+ RemovePeer(&config, to_evict);
+ EXPECT_FALSE(CanEvictReplica(config, "A", kReplicationFactor));
+ EXPECT_TRUE(IsUnderReplicated(config, kReplicationFactor));
+
+ AddPeer(&config, "E", N, '?', {{"PROMOTE", true}});
+ EXPECT_FALSE(CanEvictReplica(config, "A", kReplicationFactor));
+ EXPECT_FALSE(IsUnderReplicated(config, kReplicationFactor));
+
+ SetPeerHealth(&config, "E", '+');
+ EXPECT_FALSE(CanEvictReplica(config, "A", kReplicationFactor));
+ EXPECT_FALSE(IsUnderReplicated(config, kReplicationFactor));
+
+ PromotePeer(&config, "E");
+ ASSERT_TRUE(CanEvictReplica(config, "A", kReplicationFactor, &to_evict));
+ EXPECT_TRUE(to_evict == "B" || to_evict == "D") << to_evict;
+ EXPECT_FALSE(IsUnderReplicated(config, kReplicationFactor));
+
+ RemovePeer(&config, to_evict);
+ // The processs converges: 3 voter replicas, all are healthy.
+ EXPECT_FALSE(CanEvictReplica(config, "A", kReplicationFactor));
+ EXPECT_FALSE(IsUnderReplicated(config, kReplicationFactor));
+}
+
} // namespace consensus
} // namespace kudu
http://git-wip-us.apache.org/repos/asf/kudu/blob/2a6de8ea/src/kudu/consensus/quorum_util.cc
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/quorum_util.cc b/src/kudu/consensus/quorum_util.cc
index c544256..6e6b3c4 100644
--- a/src/kudu/consensus/quorum_util.cc
+++ b/src/kudu/consensus/quorum_util.cc
@@ -449,6 +449,8 @@ bool CanEvictReplica(const RaftConfigPB& config,
int num_voters_healthy = 0;
int num_voters_total = 0;
+ int num_voters_with_replace = 0;
+ int num_voters_viable = 0;
string non_voter_any;
string non_voter_failed;
@@ -459,6 +461,8 @@ bool CanEvictReplica(const RaftConfigPB& config,
string voter_replace;
string voter_unknown_health;
+ bool leader_with_replace = false;
+
// While working with the optional fields related to per-replica health status
// and attributes, has_a_field()-like methods are not called because of
// the appropriate default values of those fields.
@@ -475,23 +479,39 @@ bool CanEvictReplica(const RaftConfigPB& config,
switch (peer.member_type()) {
case RaftPeerPB::VOTER:
+ // A leader should always report itself as being healthy.
+ DCHECK(peer_uuid != leader_uuid || healthy) << Substitute(
+ "$0: leader reported as not healthy; config: $1",
+ peer_uuid, SecureShortDebugString(config));
+
++num_voters_total;
- if (healthy && !has_replace) {
+ if (healthy) {
++num_voters_healthy;
+ if (!has_replace) {
+ ++num_voters_viable;
+ }
+ }
+ if (has_replace) {
+ ++num_voters_with_replace;
+ if (peer_uuid == leader_uuid) {
+ leader_with_replace = true;
+ }
}
- // 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);
+ // Everything below is to keep track of replicas to evict; the leader
+ // replica is not to be evicted.
break;
}
-
if (failed && has_replace) {
voter_failed = voter_replace = peer_uuid;
}
if (failed && voter_failed.empty()) {
voter_failed = peer_uuid;
}
+ if (unknown && has_replace &&
+ (voter_replace.empty() || voter_replace != voter_failed)) {
+ voter_replace = voter_unknown_health = peer_uuid;
+ }
if (has_replace && voter_replace.empty()) {
voter_replace = peer_uuid;
}
@@ -520,33 +540,134 @@ bool CanEvictReplica(const RaftConfigPB& config,
}
}
- // An conservative approach is used when evicting replicas.
+ // A few sanity checks: the leader replica UUID should not be among those
+ // to evict.
+ DCHECK_NE(leader_uuid, voter_any);
+ DCHECK_NE(leader_uuid, voter_failed);
+ DCHECK_NE(leader_uuid, voter_replace);
+ DCHECK_NE(leader_uuid, voter_unknown_health);
+
+ // A conservative approach is used when evicting replicas. In short, the
+ // removal of replicas from the tablet without exact knowledge of their health
+ // status could lead to removing the healthy ones and keeping the failed
+ // ones, or attempting a config change operation that cannot be committed.
+ // From the other side, if the number of voter replicas in good health is
+ // greater or equal to the required replication factor, a replica with any
+ // health status can be safely evicted without compromising the availability
+ // of the tablet. Also, the eviction policy is more liberal when dealing with
+ // failed replicas: if the total number of voter replicas is greater than or
+ // equal to the required replication factor, the failed replicas are evicted
+ // aggressively. The latter is to avoid polluting the tablet servers with
+ // failed replicas, limiting the available places for new non-voter replicas
+ // created to replace failed ones. See below for more details.
+ //
+ // * A non-voter replica may be evicted regardless of its health status
+ // if the number of voter replicas in good health without the 'replace'
+ // attribute is greater than or equal to the required replication factor.
+ // The idea is to not evict non-voter replicas that might be needed to reach
+ // the required replication factor, while a present non-voter replica could
+ // be a good fit to replace a voter replica, if needed.
+ //
+ // * A non-voter replica with FAILED health status may be evicted if the
+ // number of voter replicas in good health without the 'replace' attribute
+ // is greater than or equal to a strict majority of voter replicas. The idea
+ // is to avoid polluting available tablet servers with failed non-voter
+ // replicas, while replacing failed non-voters with healthy non-voters as
+ // aggressively as possible. Also, we want to be sure that an eviction can
+ // succeed before initiating it.
//
- // A non-voter replica may be evicted only if the number of voter replicas
- // in good health without the REPLACE attribute is greater or equal to the
- // specified replication factor. The idea is to avoid evicting non-voter
- // replicas if there is uncertainty about the actual health status of the
- // voters replicas of the tablet. This is because a present non-voter replica
- // could to be a good fit to replace a voter replica which actual health
- // status is not yet known.
+ // * A voter replica may be evicted regardless of its health status
+ // if after the eviction the number of voter replicas in good health will be
+ // greater than or equal to the required replication factor and the leader
+ // replica itself is not marked with the 'replace' attribute set. The latter
+ // part of the condition emerges from the following observations:
+ // ** By definition, a voter replica marked with the 'replace' attribute
+ // should be eventually evicted from the Raft group.
+ // ** If all voter replicas are in good health and their total count is
+ // greater than the target replication and only a single one is marked
+ // with the 'replace' attribute, that's the replica to be evicted.
+ // ** Kudu Raft implementation does not support evicting the leader of
+ // a Raft group.
+ // So, the removal of a leader replica marked with the 'replace' attribute
+ // is postponed until the leader replica steps down and becomes a follower.
//
- // A voter replica may be evicted only if the number of voter replicas in good
- // health without the REPLACE attribute is greater or equal to the specified
- // replication factor. Removal of voter replicas from the tablet without exact
- // knowledge of their health status could lead to removing the healthy ones
- // and keeping the failed ones.
- const bool can_evict_non_voter =
- num_voters_healthy >= replication_factor && num_non_voters_total > 0;
- const bool can_evict_voter =
- num_voters_healthy > replication_factor ||
- (num_voters_healthy == replication_factor &&
- num_voters_total > replication_factor);
+ // * A voter replica with FAILED health may be evicted only if the total
+ // number of voter replicas is greater than the required replication factor
+ // and the number of *other* voter replicas in good health without the
+ // 'replace' attribute is greater than or equal to a strict majority of
+ // voter replicas.
+ //
+ // * A voter replica in good health marked with the 'replace' attribute may be
+ // evicted when the number of replicas in good health after the eviction
+ // is greater than or equal to the required replication factor.
+
+ bool need_to_evict_non_voter = false;
+
+ // Check if there is any excess non-voter replica. We add non-voter replicas
+ // to replace non-viable (i.e. failed or explicitly marked for eviction) ones.
+ need_to_evict_non_voter |=
+ num_voters_viable >= replication_factor &&
+ num_non_voters_total > 0;
+
+ // Some non-voter replica has failed: we want to remove those aggressively.
+ // This is to avoid polluting tablet servers with failed replicas. Otherwise,
+ // it may be a situation when it's impossible to add a new non-voter replica
+ // to replace failed ones.
+ need_to_evict_non_voter |= !non_voter_failed.empty();
+
+ // All the non-voter-related sub-cases are applicable only when there is at
+ // least one non-voter replica and a majority of voter replicas are on-line
+ // to commit the Raft configuration change.
+ const bool can_evict_non_voter = need_to_evict_non_voter &&
+ num_voters_healthy >= MajoritySize(num_voters_total);
+
+ bool need_to_evict_voter = false;
+
+ // The abundant case: can evict any voter replica. The code below will select
+ // the most appropriate candidate.
+ need_to_evict_voter |= num_voters_viable > replication_factor;
+
+ // Some voter replica has failed: we want to remove those aggressively.
+ // This is to avoid polluting tablet servers with failed replicas. Otherwise,
+ // it may be a situation when it's impossible to add a new non-voter replica
+ // to replace failed ones.
+ need_to_evict_voter |= !voter_failed.empty();
+
+ // In case if we already have enough healthy replicas running, it's safe to
+ // get rid of replicas with unknown health state.
+ need_to_evict_voter |=
+ num_voters_viable >= replication_factor &&
+ !voter_unknown_health.empty();
+
+ // Working with the replicas marked with the 'replace' attribute:
+ // the case when too many replicas are marked with the 'replace' attribute
+ // while all required replicas are healthy.
+ need_to_evict_voter |= (num_voters_healthy >= replication_factor) &&
+ ((num_voters_with_replace > replication_factor) ||
+ (num_voters_with_replace >= replication_factor && num_voters_viable > 0));
+
+ // Working with the replicas marked with the 'replace' attribute:
+ // the case where a few replicas are marked with the 'replace' attribute
+ // while all required replicas are healthy.
+ //
+ // In the special case when the leader replica is the only one marked with the
+ // 'replace' attribute, the leader replica cannot be evicted.
+ need_to_evict_voter |=
+ !(num_voters_with_replace == 1 && leader_with_replace) &&
+ (num_voters_with_replace > 0 && num_voters_healthy > replication_factor);
+
+ // The voter-related sub-cases are applicable only when the total number of
+ // voter replicas is greater than the target replication factor and a majority
+ // of voter replicas are on-line to commit the Raft configuration change.
+ const bool can_evict_voter = need_to_evict_voter &&
+ num_voters_total > replication_factor &&
+ num_voters_healthy >= MajoritySize(num_voters_total - 1);
const bool can_evict = can_evict_non_voter || can_evict_voter;
string to_evict;
if (can_evict_non_voter) {
- // First try to evict an excess non-voter, if present. This is to avoid
- // possible IO load due to tablet copy in progress.
+ // First try to evict an excess non-voter, if present. This might help to
+ // avoid IO load due to a tablet copy in progress.
//
// Non-voter candidates for eviction (in decreasing priority):
// * failed
@@ -564,14 +685,14 @@ bool CanEvictReplica(const RaftConfigPB& config,
//
// Voter candidates for eviction (in decreasing priority):
// * failed and having the attribute REPLACE set
- // * having the attribute REPLACE set
// * failed
+ // * having the attribute REPLACE set
// * in unknown health state
// * any other
- if (!voter_replace.empty()) {
- to_evict = voter_replace;
- } else if (!voter_failed.empty()) {
+ if (!voter_failed.empty()) {
to_evict = voter_failed;
+ } else if (!voter_replace.empty()) {
+ to_evict = voter_replace;
} else if (!voter_unknown_health.empty()) {
to_evict = voter_unknown_health;
} else {
@@ -581,6 +702,7 @@ bool CanEvictReplica(const RaftConfigPB& config,
if (can_evict) {
DCHECK(!to_evict.empty());
+ DCHECK_NE(leader_uuid, to_evict);
if (uuid_to_evict) {
*uuid_to_evict = to_evict;
}
@@ -588,6 +710,7 @@ bool CanEvictReplica(const RaftConfigPB& config,
VLOG(2) << "decision: can"
<< (can_evict ? "" : "not") << " evict replica "
<< (can_evict ? to_evict : "");
+
return can_evict;
}
http://git-wip-us.apache.org/repos/asf/kudu/blob/2a6de8ea/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
----------------------------------------------------------------------
diff --git a/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc b/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
index f3a0d9e..55e3395 100644
--- a/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
+++ b/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
@@ -1385,7 +1385,7 @@ TEST_F(RaftConsensusNonVoterITest, TabletServerIsGoneAndBack) {
NO_FATALS(cluster_->AssertNoCrashes());
}
-// A two-step sceanario: first, an existing tablet replica fails because it's
+// A two-step sceanario: first, an existing tablet replica fails because it
// fails behind the threshold of the GCed WAL segment threshold. The catalog
// manager should notice that and add a new non-voter replica in attempt to
// replace the failed replica. Then, the newly added non-voter replica becomes
@@ -1445,6 +1445,7 @@ TEST_F(RaftConsensusNonVoterITest, FailedTabletCopy) {
ts1->Shutdown();
SleepFor(kConsensusRpcTimeout);
+ // Cause follower 'follower_uuid' to fail.
string follower_uuid;
{
TabletServerMap replica_servers;
@@ -1460,7 +1461,7 @@ TEST_F(RaftConsensusNonVoterITest, FailedTabletCopy) {
}
// The leader replica marks the non-responsive replica as failed after it
- // realized the replica would not be able to catch up, and the catalog
+ // realizes the replica would not be able to catch up, and the catalog
// manager should add a new non-voter as a replacement.
bool has_leader = false;
TabletLocationsPB tablet_locations;
@@ -1472,11 +1473,12 @@ TEST_F(RaftConsensusNonVoterITest, FailedTabletCopy) {
ANY_REPLICA,
&has_leader,
&tablet_locations));
- // Make the second server available for placing a non-voter replica.
- ASSERT_OK(ts1->Restart());
- // The system should be able to recover, keeping only necessary number of
- // voter replicas and purging the failed voters and non-voters.
+ // However, the tablet server with the new non-voter replica crashes during
+ // the tablet copy phase. Give the catalog manager some time to detect that
+ // and purge the failed non-voter from the configuration. Also, the TS manager
+ // should detect that the crashed server is not a candidate for new replicas
+ // anymore.
ASSERT_OK(WaitForReplicasReportedToMaster(cluster_->master_proxy(),
kReplicasNum,
tablet_id_,
@@ -1493,19 +1495,59 @@ TEST_F(RaftConsensusNonVoterITest, FailedTabletCopy) {
// have changed in between of these two calls.
ASSERT_OK(GetConsensusState(leader, tablet_id_, kTimeout, &cstate));
});
- // The original voter replica fallen behind WAL catchup threshold should be evicted.
- EXPECT_FALSE(IsRaftConfigMember(follower_uuid, cstate.committed_config()))
+ // The original voter replica that fell behind WAL catchup threshold should
+ // still be there.
+ EXPECT_TRUE(IsRaftConfigMember(follower_uuid, cstate.committed_config()))
<< pb_util::SecureDebugString(cstate.committed_config())
- << "fallen behind WAL replica UUID: " << follower_uuid;
+ << "fell behind WAL replica UUID: " << follower_uuid;
// The first non-voter replica failed on tablet copy should be evicted.
EXPECT_FALSE(IsRaftConfigMember(ts0->uuid(), cstate.committed_config()))
<< pb_util::SecureDebugString(cstate.committed_config())
<< "failed tablet copy replica UUID: " << ts0->uuid();
- // The tablet copy on the restarted server should succeed and this replica
- // should replace the original failed replica.
- EXPECT_TRUE(IsRaftConfigMember(ts1->uuid(), cstate.committed_config()))
+ // No replicas from the second tablet server should be in the config yet.
+ EXPECT_FALSE(IsRaftConfigMember(ts1->uuid(), cstate.committed_config()))
<< pb_util::SecureDebugString(cstate.committed_config())
<< "new replacement replica UUID: " << ts1->uuid();
+
+ // Make the second server available for placing a non-voter replica. Tablet
+ // copy for replicas on this with this server should succeed.
+ ASSERT_OK(ts1->Restart());
+
+ // The system should be able to recover, replacing the failed replica with
+ // the replica on ts1. Since it's hard to predict when the replacement takes
+ // place, the easiest way is to wait until it's converges into the required
+ // state.
+ ASSERT_EVENTUALLY([&] {
+ bool has_leader = false;
+ TabletLocationsPB tablet_locations;
+ ASSERT_OK(WaitForReplicasReportedToMaster(cluster_->master_proxy(),
+ kReplicasNum,
+ tablet_id_,
+ kTimeout,
+ WAIT_FOR_LEADER,
+ ANY_REPLICA,
+ &has_leader,
+ &tablet_locations));
+
+ TServerDetails* leader = nullptr;
+ ASSERT_OK(GetLeaderReplicaWithRetries(tablet_id_, &leader));
+ ASSERT_OK(GetConsensusState(leader, tablet_id_, kTimeout, &cstate));
+ // The original voter replica that fell behind the WAL catchup threshold
+ // should be evicted.
+ ASSERT_FALSE(IsRaftConfigMember(follower_uuid, cstate.committed_config()))
+ << pb_util::SecureDebugString(cstate.committed_config())
+ << "fell behind WAL replica UUID: " << follower_uuid;
+ // The first non-voter replica failed during the tablet copy phase
+ // should not be present.
+ ASSERT_FALSE(IsRaftConfigMember(ts0->uuid(), cstate.committed_config()))
+ << pb_util::SecureDebugString(cstate.committed_config())
+ << "failed tablet copy replica UUID: " << ts0->uuid();
+ // The tablet copy on the restarted server should succeed and this replica
+ // should replace the original failed replica.
+ ASSERT_TRUE(IsRaftConfigMember(ts1->uuid(), cstate.committed_config()))
+ << pb_util::SecureDebugString(cstate.committed_config())
+ << "new replacement replica UUID: " << ts1->uuid();
+ });
}
} // namespace tserver
http://git-wip-us.apache.org/repos/asf/kudu/blob/2a6de8ea/src/kudu/master/catalog_manager.cc
----------------------------------------------------------------------
diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc
index 10e0f37..2b7bcc4 100644
--- a/src/kudu/master/catalog_manager.cc
+++ b/src/kudu/master/catalog_manager.cc
@@ -3507,9 +3507,13 @@ Status CatalogManager::ProcessTabletReport(
// When --raft_prepare_replacement_before_eviction is enabled, we
// consider whether to add or evict replicas based on the health report
// included in the leader's tablet report. Since only the leader tracks
- // health, we ignore reports from non-leaders in this case.
- } else if (!cstate.leader_uuid().empty() &&
- ts_desc->permanent_uuid() == cstate.leader_uuid()) {
+ // health, we ignore reports from non-leaders in this case. Also, making
+ // the changes recommended by CanEvictReplica()/IsUnderReplicated()
+ // assumes that the leader replica has already committed the configuration
+ // it's working with.
+ } else if (!cstate.has_pending_config() &&
+ !cstate.leader_uuid().empty() &&
+ cstate.leader_uuid() == ts_desc->permanent_uuid()) {
const RaftConfigPB& config = cstate.committed_config();
string to_evict;
if (PREDICT_TRUE(FLAGS_catalog_manager_evict_excess_replicas) &&
http://git-wip-us.apache.org/repos/asf/kudu/blob/2a6de8ea/src/kudu/tools/kudu-admin-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tools/kudu-admin-test.cc b/src/kudu/tools/kudu-admin-test.cc
index 5b7afaf..fe91dd9 100644
--- a/src/kudu/tools/kudu-admin-test.cc
+++ b/src/kudu/tools/kudu-admin-test.cc
@@ -315,7 +315,7 @@ TEST_F(AdminCliTest, TestMoveTablet_pre_KUDU_1097) {
DoTestMoveTablet(kDisableKudu1097);
}
-TEST_F(AdminCliTest, DISABLED_TestMoveTablet_KUDU_1097) {
+TEST_F(AdminCliTest, TestMoveTablet_KUDU_1097) {
DoTestMoveTablet(kEnableKudu1097);
}