You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by gr...@apache.org on 2018/05/21 17:02:31 UTC
kudu git commit: [consensus] KUDU-2443 fix replica replacement of RF=1
Repository: kudu
Updated Branches:
refs/heads/branch-1.7.x 5ef577b65 -> 5bd204c1a
[consensus] KUDU-2443 fix replica replacement of RF=1
Fixed bug in the consensus::ShouldEvictReplica() function for the case
when the leader replica of a tablet with replication factor of 1
is marked with the REPLACE attribute and there is an extra voter
replica in the tablet's Raft config.
Prior to this fix, the master would evict the extra voter from the
configuration and then it would add a new non-voter because of the
consensus::ShouldAddReplica() method's behavior. After the newly
added non-voter replica catches up and becomes a voter, that would
happen again and again, until the REPLACE attribute is removed.
This changelist also includes regression tests to cover the
corresponding functionality. Also, additional test scenarios added
to extend the coverage for the single-replica edge case.
Change-Id: I9da9fe6788f28b40f7adc53e23540bcdf103c1ea
Reviewed-on: http://gerrit.cloudera.org:8080/10438
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy <mp...@apache.org>
(cherry picked from commit cb203729255dba06120d2a2f8702032a2ffd9694)
Reviewed-on: http://gerrit.cloudera.org:8080/10464
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/5bd204c1
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/5bd204c1
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/5bd204c1
Branch: refs/heads/branch-1.7.x
Commit: 5bd204c1a83770f2a628565ab997026b574e35bd
Parents: 5ef577b
Author: Alexey Serbin <as...@cloudera.com>
Authored: Wed May 16 17:35:31 2018 -0700
Committer: Grant Henke <gr...@apache.org>
Committed: Mon May 21 16:55:43 2018 +0000
----------------------------------------------------------------------
src/kudu/consensus/quorum_util-test.cc | 83 ++++++++++++++++++++++++++++-
src/kudu/consensus/quorum_util.cc | 4 ++
2 files changed, 86 insertions(+), 1 deletion(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/5bd204c1/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 f98e819..46776e0 100644
--- a/src/kudu/consensus/quorum_util-test.cc
+++ b/src/kudu/consensus/quorum_util-test.cc
@@ -15,7 +15,10 @@
// specific language governing permissions and limitations
// under the License.
+#include "kudu/consensus/quorum_util.h"
+
#include <memory>
+#include <ostream>
#include <string>
#include <utility>
#include <vector>
@@ -25,7 +28,6 @@
#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"
@@ -73,6 +75,21 @@ static void SetOverallHealth(HealthReportPB* health_report,
}
}
+std::ostream& operator<<(std::ostream& os, MajorityHealthPolicy policy) {
+ switch (policy) {
+ case MajorityHealthPolicy::HONOR:
+ os << "MajorityHealthPolicy::HONOR";
+ break;
+ case MajorityHealthPolicy::IGNORE:
+ os << "MajorityHealthPolicy::IGNORE";
+ break;
+ default:
+ os << policy << ": unsupported health policy";
+ break;
+ }
+ return os;
+}
+
// Add a consensus peer into the specified configuration.
static void AddPeer(RaftConfigPB* config,
const string& uuid,
@@ -1057,6 +1074,62 @@ TEST_P(QuorumUtilHealthPolicyParamTest, ReplaceAttributeBasic) {
{
RaftConfigPB config;
AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ EXPECT_TRUE(ShouldAddReplica(config, 1, policy));
+ EXPECT_FALSE(ShouldEvictReplica(config, "A", 1, policy));
+ }
+ {
+ // Regression test scenario for KUDU-2443.
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+');
+ EXPECT_FALSE(ShouldAddReplica(config, 1, policy));
+ EXPECT_FALSE(ShouldEvictReplica(config, "A", 1, policy));
+ string to_evict;
+ ASSERT_TRUE(ShouldEvictReplica(config, "B", 1, policy, &to_evict));
+ EXPECT_EQ("A", to_evict);
+ }
+ {
+ for (auto health_status : { '+', '-', '?', 'x' }) {
+ SCOPED_TRACE(Substitute("health status '$0'", health_status));
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", N, health_status);
+ EXPECT_TRUE(ShouldAddReplica(config, 1, policy));
+ if (health_status == '+' || health_status == '?') {
+ EXPECT_FALSE(ShouldEvictReplica(config, "A", 1, policy));
+ } else {
+ string to_evict;
+ ASSERT_TRUE(ShouldEvictReplica(config, "A", 1, policy, &to_evict));
+ EXPECT_EQ("B", to_evict);
+ }
+ }
+ }
+ // If a non-voter replica with PROMOTE=true is already in the Raft config,
+ // no need to add an additional one if the health status of the non-voter
+ // replica is HEALTHY or UNKNOWN.
+ {
+ for (auto health_status : { '+', '-', '?', 'x' }) {
+ SCOPED_TRACE(Substitute("health status '$0'", health_status));
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", N, health_status, {{"PROMOTE", true}});
+ if (health_status == '+' || health_status == '?') {
+ EXPECT_FALSE(ShouldAddReplica(config, 1, policy));
+ } else {
+ EXPECT_TRUE(ShouldAddReplica(config, 1, policy));
+ }
+ if (health_status == '+' || health_status == '?') {
+ EXPECT_FALSE(ShouldEvictReplica(config, "A", 1, policy));
+ } else {
+ string to_evict;
+ ASSERT_TRUE(ShouldEvictReplica(config, "A", 1, policy, &to_evict));
+ EXPECT_EQ("B", to_evict);
+ }
+ }
+ }
+ {
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
AddPeer(&config, "B", V, '+');
AddPeer(&config, "C", V, '+');
EXPECT_FALSE(ShouldEvictReplica(config, "A", 3, policy));
@@ -1070,6 +1143,13 @@ TEST_P(QuorumUtilHealthPolicyParamTest, ReplaceAttributeBasic) {
AddPeer(&config, "D", V, '+');
EXPECT_FALSE(ShouldEvictReplica(config, "A", 3, policy));
EXPECT_FALSE(ShouldAddReplica(config, 3, policy));
+
+ for (const auto& leader_replica : { "B", "C", "D" }) {
+ string to_evict;
+ SCOPED_TRACE(Substitute("leader $0", leader_replica));
+ ASSERT_TRUE(ShouldEvictReplica(config, leader_replica, 3, policy, &to_evict));
+ EXPECT_EQ("A", to_evict);
+ }
}
for (auto health_status : { '-', '?', 'x' }) {
RaftConfigPB config;
@@ -1093,6 +1173,7 @@ TEST_P(QuorumUtilHealthPolicyParamTest, ReplaceAttributeBasic) {
AddPeer(&config, "C", V, '+');
AddPeer(&config, "D", V, '+');
AddPeer(&config, "E", V, '+');
+ // There should be no attempt to evict the leader.
string to_evict;
ASSERT_TRUE(ShouldEvictReplica(config, "A", 3, policy, &to_evict));
EXPECT_NE("A", to_evict);
http://git-wip-us.apache.org/repos/asf/kudu/blob/5bd204c1/src/kudu/consensus/quorum_util.cc
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/quorum_util.cc b/src/kudu/consensus/quorum_util.cc
index 1c0fc71..88a231c 100644
--- a/src/kudu/consensus/quorum_util.cc
+++ b/src/kudu/consensus/quorum_util.cc
@@ -732,7 +732,11 @@ bool ShouldEvictReplica(const RaftConfigPB& config,
// 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.
+ //
+ // 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_healthy >= replication_factor) &&
+ !(num_voters_with_replace == 1 && leader_with_replace) &&
((num_voters_with_replace > replication_factor) ||
(num_voters_with_replace >= replication_factor && num_voters_viable > 0));