kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From mpe...@apache.org
Subject [2/2] kudu git commit: [consensus] KUDU-2367 fix replica health report
Date Sat, 24 Mar 2018 02:26:39 GMT
[consensus] KUDU-2367 fix replica health report

This patch addresses KUDU-2367,where a leader replica under particular
conditions reports follower's health status as FAILED instead of
FAILED_UNRECOVERABLE. I.e., the bug in the code of the
PeerMessageQueue::PeerHealthStatus() method resulted in having the
leader reporting the health status of the replica still as FAILED even
after the replica fell behind the WAL segment GC threshold.

Also, a couple of already existing integration tests are updated to
provide appropriate coverage for this patch.

Change-Id: If77474adc8f618f2cda35f992190133138fdb511
Reviewed-on: http://gerrit.cloudera.org:8080/9755
Tested-by: Alexey Serbin <aserbin@cloudera.com>
Reviewed-by: Mike Percy <mpercy@apache.org>
Tested-by: Kudu Jenkins
(cherry picked from commit fcb0be6381a47155e171eb50a333af502bbf506f)
Reviewed-on: http://gerrit.cloudera.org:8080/9786


Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/310dbe05
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/310dbe05
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/310dbe05

Branch: refs/heads/branch-1.7.x
Commit: 310dbe05371d955704839fb31b7ad868589cfc0a
Parents: b5e990b
Author: Alexey Serbin <aserbin@cloudera.com>
Authored: Thu Mar 22 01:02:32 2018 -0700
Committer: Alexey Serbin <aserbin@cloudera.com>
Committed: Sat Mar 24 01:18:52 2018 +0000

----------------------------------------------------------------------
 src/kudu/consensus/consensus_queue-test.cc      | 67 +++++++++++++++++++-
 src/kudu/consensus/consensus_queue.cc           | 42 +++++++++---
 src/kudu/consensus/consensus_queue.h            |  1 +
 .../raft_consensus-itest-base.cc                |  7 +-
 .../raft_consensus-itest-base.h                 |  5 +-
 .../raft_consensus_nonvoter-itest.cc            | 40 ++++++++----
 6 files changed, 137 insertions(+), 25 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/310dbe05/src/kudu/consensus/consensus_queue-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/consensus_queue-test.cc b/src/kudu/consensus/consensus_queue-test.cc
index c8dc086..c3cfd24 100644
--- a/src/kudu/consensus/consensus_queue-test.cc
+++ b/src/kudu/consensus/consensus_queue-test.cc
@@ -59,9 +59,9 @@
 #include "kudu/util/threadpool.h"
 
 DECLARE_int32(consensus_max_batch_size_bytes);
+DECLARE_int32(follower_unavailable_considered_failed_sec);
 
-METRIC_DECLARE_entity(tablet);
-
+using kudu::consensus::HealthReportPB;
 using std::vector;
 
 namespace kudu {
@@ -189,7 +189,7 @@ class ConsensusQueueTest : public KuduTest {
 
   void WaitForLocalPeerToAckIndex(int index) {
     while (true) {
-      PeerMessageQueue::TrackedPeer leader = queue_->GetTrackedPeerForTests(kLeaderUuid);
+      const auto leader = queue_->GetTrackedPeerForTests(kLeaderUuid);
       if (leader.last_received.index() >= index) {
         break;
       }
@@ -945,5 +945,66 @@ TEST_F(ConsensusQueueTest, TestFollowerCommittedIndexAndMetrics) {
   ASSERT_EQ(5, queue_->metrics_.num_ops_behind_leader->value());
 }
 
+// Unit test for the PeerMessageQueue::PeerHealthStatus() method.
+TEST(ConsensusQueueUnitTest, PeerHealthStatus) {
+  static constexpr PeerStatus kPeerStatusesForUnknown[] = {
+     PeerStatus::NEW,
+     PeerStatus::REMOTE_ERROR,
+     PeerStatus::RPC_LAYER_ERROR,
+     PeerStatus::TABLET_NOT_FOUND,
+     PeerStatus::INVALID_TERM,
+     PeerStatus::CANNOT_PREPARE,
+     PeerStatus::LMP_MISMATCH,
+  };
+
+  RaftPeerPB peer_pb;
+  PeerMessageQueue::TrackedPeer peer(peer_pb);
+  EXPECT_EQ(HealthReportPB::UNKNOWN, PeerMessageQueue::PeerHealthStatus(peer));
+  for (auto status : kPeerStatusesForUnknown) {
+    peer.last_exchange_status = status;
+    EXPECT_EQ(HealthReportPB::UNKNOWN, PeerMessageQueue::PeerHealthStatus(peer));
+  }
+
+  peer.last_exchange_status = PeerStatus::TABLET_FAILED;
+  EXPECT_EQ(HealthReportPB::FAILED_UNRECOVERABLE, PeerMessageQueue::PeerHealthStatus(peer));
+
+  peer.last_exchange_status = PeerStatus::OK;
+  EXPECT_EQ(HealthReportPB::HEALTHY, PeerMessageQueue::PeerHealthStatus(peer));
+
+  peer.wal_catchup_possible = false;
+  EXPECT_EQ(HealthReportPB::FAILED_UNRECOVERABLE, PeerMessageQueue::PeerHealthStatus(peer));
+
+  peer.wal_catchup_possible = true;
+  EXPECT_EQ(HealthReportPB::HEALTHY, PeerMessageQueue::PeerHealthStatus(peer));
+
+  peer.last_communication_time -=
+      MonoDelta::FromSeconds(FLAGS_follower_unavailable_considered_failed_sec + 1);
+  EXPECT_EQ(HealthReportPB::FAILED, PeerMessageQueue::PeerHealthStatus(peer));
+  for (auto status : kPeerStatusesForUnknown) {
+    peer.last_exchange_status = status;
+    EXPECT_EQ(HealthReportPB::FAILED, PeerMessageQueue::PeerHealthStatus(peer));
+  }
+
+  peer.last_exchange_status = PeerStatus::TABLET_FAILED;
+  EXPECT_EQ(HealthReportPB::FAILED_UNRECOVERABLE, PeerMessageQueue::PeerHealthStatus(peer));
+
+  peer.last_exchange_status = PeerStatus::OK;
+  EXPECT_EQ(HealthReportPB::FAILED, PeerMessageQueue::PeerHealthStatus(peer));
+
+  peer.wal_catchup_possible = false;
+  EXPECT_EQ(HealthReportPB::FAILED_UNRECOVERABLE, PeerMessageQueue::PeerHealthStatus(peer));
+
+  for (auto status : kPeerStatusesForUnknown) {
+    peer.last_exchange_status = status;
+    EXPECT_EQ(HealthReportPB::FAILED_UNRECOVERABLE, PeerMessageQueue::PeerHealthStatus(peer));
+  }
+
+  peer.last_exchange_status = PeerStatus::OK;
+  EXPECT_EQ(HealthReportPB::FAILED_UNRECOVERABLE, PeerMessageQueue::PeerHealthStatus(peer));
+
+  peer.last_exchange_status = PeerStatus::TABLET_FAILED;
+  EXPECT_EQ(HealthReportPB::FAILED_UNRECOVERABLE, PeerMessageQueue::PeerHealthStatus(peer));
+}
+
 }  // namespace consensus
 }  // namespace kudu

http://git-wip-us.apache.org/repos/asf/kudu/blob/310dbe05/src/kudu/consensus/consensus_queue.cc
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/consensus_queue.cc b/src/kudu/consensus/consensus_queue.cc
index 804739c..63ffd74 100644
--- a/src/kudu/consensus/consensus_queue.cc
+++ b/src/kudu/consensus/consensus_queue.cc
@@ -552,24 +552,48 @@ void PeerMessageQueue::UpdatePeerHealthUnlocked(TrackedPeer* peer) {
   }
 }
 
+// While reporting on the replica health status, it's important to report on
+// the 'definitive' health statuses once they surface. That allows the system
+// to expedite decisions on replica replacement because the more 'definitive'
+// statuses have less uncertainty and provide more information (compared
+// with less 'definitive' statuses). Informally, the level of 'definitiveness'
+// could be measured by the number of possible state transitions on the replica
+// health status state diagram.
+//
+// The health status chain below has increasing level of 'definitiveness'
+// left to right:
+//
+//   UNKNOWN --> HEALTHY --> FAILED --> FAILED_UNRECOVERABLE
+//
+// For example, in the case when a replica has been unreachable longer than the
+// time interval specified by the --follower_unavailable_considered_failed_sec
+// flag, the system should start reporting its health status as FAILED.
+// However, once the replica falls behind the WAL log GC threshold, the system
+// should start reporting its healths status as FAILED_UNRECOVERABLE. The code
+// below is written to adhere to that informal policy.
 HealthReportPB::HealthStatus PeerMessageQueue::PeerHealthStatus(const TrackedPeer& peer)
{
-  // Replicas which have been unreachable for too long are considered failed.
-  auto max_unreachable = MonoDelta::FromSeconds(FLAGS_follower_unavailable_considered_failed_sec);
-  if (MonoTime::Now() - peer.last_communication_time > max_unreachable) {
-    return HealthReportPB::FAILED;
-  }
-
-  // Replicas that have fallen behind the leader's retained WAL are failed
-  // and have no chance to come back.
+  // Replicas that have fallen behind the leader's retained WAL segments are
+  // failed irrecoverably and will not come back because they cannot ever catch
+  // up with the leader replica.
   if (!peer.wal_catchup_possible) {
     return HealthReportPB::FAILED_UNRECOVERABLE;
   }
 
-  // Replicas returning TABLET_FAILED status are considered failed.
+  // Replicas returning TABLET_FAILED status are considered irrecoverably
+  // failed because the TABLED_FAILED status manifests about IO failures
+  // caused by disk corruption, etc.
   if (peer.last_exchange_status == PeerStatus::TABLET_FAILED) {
     return HealthReportPB::FAILED_UNRECOVERABLE;
   }
 
+  // Replicas which have been unreachable for too long are considered failed,
+  // unless it's known that they have failed irrecoverably (see above). They
+  // might come back at some point and successfully catch up with the leader.
+  auto max_unreachable = MonoDelta::FromSeconds(FLAGS_follower_unavailable_considered_failed_sec);
+  if (MonoTime::Now() - peer.last_communication_time > max_unreachable) {
+    return HealthReportPB::FAILED;
+  }
+
   // The happy case: replicas returned OK during the recent exchange are considered healthy.
   if (peer.last_exchange_status == PeerStatus::OK) {
     return HealthReportPB::HEALTHY;

http://git-wip-us.apache.org/repos/asf/kudu/blob/310dbe05/src/kudu/consensus/consensus_queue.h
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/consensus_queue.h b/src/kudu/consensus/consensus_queue.h
index cbbc522..a2b79e2 100644
--- a/src/kudu/consensus/consensus_queue.h
+++ b/src/kudu/consensus/consensus_queue.h
@@ -360,6 +360,7 @@ class PeerMessageQueue {
   FRIEND_TEST(ConsensusQueueTest, TestQueueAdvancesCommittedIndex);
   FRIEND_TEST(ConsensusQueueTest, TestQueueMovesWatermarksBackward);
   FRIEND_TEST(ConsensusQueueTest, TestFollowerCommittedIndexAndMetrics);
+  FRIEND_TEST(ConsensusQueueUnitTest, PeerHealthStatus);
   FRIEND_TEST(RaftConsensusQuorumTest, TestReplicasEnforceTheLogMatchingProperty);
 
   // Mode specifies how the queue currently behaves:

http://git-wip-us.apache.org/repos/asf/kudu/blob/310dbe05/src/kudu/integration-tests/raft_consensus-itest-base.cc
----------------------------------------------------------------------
diff --git a/src/kudu/integration-tests/raft_consensus-itest-base.cc b/src/kudu/integration-tests/raft_consensus-itest-base.cc
index aab6981..1fee8a8 100644
--- a/src/kudu/integration-tests/raft_consensus-itest-base.cc
+++ b/src/kudu/integration-tests/raft_consensus-itest-base.cc
@@ -197,7 +197,8 @@ void RaftConsensusITestBase::CauseFollowerToFallBehindLogGC(
     string* leader_uuid,
     int64_t* orig_term,
     string* fell_behind_uuid,
-    BehindWalGcBehavior tserver_behavior) {
+    BehindWalGcBehavior tserver_behavior,
+    const MonoDelta& pre_workload_delay) {
   MonoDelta kTimeout = MonoDelta::FromSeconds(10);
   // Wait for all of the replicas to have acknowledged the elected
   // leader and logged the first NO_OP.
@@ -234,6 +235,10 @@ void RaftConsensusITestBase::CauseFollowerToFallBehindLogGC(
   *leader_uuid = leader->uuid();
   int leader_index = cluster_->tablet_server_index_by_uuid(*leader_uuid);
 
+  if (pre_workload_delay.Initialized()) {
+    SleepFor(pre_workload_delay);
+  }
+
   TestWorkload workload(cluster_.get());
   workload.set_table_name(kTableId);
   workload.set_timeout_allowed(true);

http://git-wip-us.apache.org/repos/asf/kudu/blob/310dbe05/src/kudu/integration-tests/raft_consensus-itest-base.h
----------------------------------------------------------------------
diff --git a/src/kudu/integration-tests/raft_consensus-itest-base.h b/src/kudu/integration-tests/raft_consensus-itest-base.h
index a4266ad..c00426d 100644
--- a/src/kudu/integration-tests/raft_consensus-itest-base.h
+++ b/src/kudu/integration-tests/raft_consensus-itest-base.h
@@ -28,6 +28,8 @@
 
 namespace kudu {
 
+class MonoDelta;
+
 namespace cluster {
 class ExternalTabletServer;
 }
@@ -84,7 +86,8 @@ class RaftConsensusITestBase : public TabletServerIntegrationTestBase {
       std::string* leader_uuid,
       int64_t* orig_term,
       std::string* fell_behind_uuid,
-      BehindWalGcBehavior tserver_behavior = BehindWalGcBehavior::STOP_CONTINUE);
+      BehindWalGcBehavior tserver_behavior = BehindWalGcBehavior::STOP_CONTINUE,
+      const MonoDelta& pre_workload_delay = {});
 
   CountDownLatch inserters_;
 };

http://git-wip-us.apache.org/repos/asf/kudu/blob/310dbe05/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 84a6d94..da19cc4 100644
--- a/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
+++ b/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
@@ -1927,20 +1927,28 @@ TEST_P(IncompatibleReplicaReplacementSchemesITest, MasterAndTserverMisconfig)
{
   }
 }
 
+// This test scenario runs the system with the 3-4-3 replica management scheme
+// having the total number of tablet servers equal to the replication factor
+// of the tablet being tested. The scenario makes one follower replica
+// fall behind the WAL segment GC threshold. The system should be able to
+// replace the failed replica 'in-place', i.e. no additional tablet server is
+// needed for the cluster to recover in such situations. The scenario verifies
+// that the re-replication works as expected when the tablet server hosting the
+// failed replica is:
+//   ** paused and then resumed, emulating a lagging tablet server
+//   ** shut down and then started back up
 class ReplicaBehindWalGcThresholdITest :
     public RaftConsensusNonVoterITest,
-    public ::testing::WithParamInterface<RaftConsensusITestBase::BehindWalGcBehavior>
{
+    public ::testing::WithParamInterface<
+        std::tuple<RaftConsensusITestBase::BehindWalGcBehavior, bool>> {
 };
 INSTANTIATE_TEST_CASE_P(,
     ReplicaBehindWalGcThresholdITest,
-    ::testing::Values(RaftConsensusITestBase::BehindWalGcBehavior::STOP_CONTINUE,
-                      RaftConsensusITestBase::BehindWalGcBehavior::SHUTDOWN_RESTART,
-                      RaftConsensusITestBase::BehindWalGcBehavior::SHUTDOWN));
-
-// Test that the catalog manager running with the 3-4-3 scheme is able to do
-// 'in-place' replica replacement when replica falls behind the WAL segment GC,
-// i.e. no additional tablet server is needed in case of tablet with replication
-// factor 3 when there are just 3 tablet servers in the cluster.
+    ::testing::Combine(
+        ::testing::Values(RaftConsensusITestBase::BehindWalGcBehavior::STOP_CONTINUE,
+                          RaftConsensusITestBase::BehindWalGcBehavior::SHUTDOWN_RESTART,
+                          RaftConsensusITestBase::BehindWalGcBehavior::SHUTDOWN),
+        ::testing::Bool()));
 TEST_P(ReplicaBehindWalGcThresholdITest, ReplicaReplacement) {
   if (!AllowSlowTests()) {
     LOG(WARNING) << "test is skipped; set KUDU_ALLOW_SLOW_TESTS=1 to run";
@@ -1953,7 +1961,8 @@ TEST_P(ReplicaBehindWalGcThresholdITest, ReplicaReplacement) {
   const auto kUnavaiableFailedSec = 5;
   FLAGS_num_replicas = kReplicasNum;
   FLAGS_num_tablet_servers = kReplicasNum;
-  const auto tserver_behavior = GetParam();
+  const auto tserver_behavior = std::get<0>(GetParam());
+  const bool do_delay_workload = std::get<1>(GetParam());
 
   vector<string> master_flags = {
     // This scenario runs with the 3-4-3 replica management scheme.
@@ -1981,8 +1990,17 @@ TEST_P(ReplicaBehindWalGcThresholdITest, ReplicaReplacement) {
   string follower_uuid;
   string leader_uuid;
   int64_t orig_term;
+  MonoDelta delay;
+  if (do_delay_workload) {
+    // That's to make the leader replica to report the state of the tablet as
+    // FAILED first. Later on, when the replica falls behind the WAL segment GC
+    // threshold, the leader replica should report the follower's health status
+    // as FAILED_UNRECOVERABLE.
+    delay = MonoDelta::FromSeconds(3 * kUnavaiableFailedSec);
+  }
   NO_FATALS(CauseFollowerToFallBehindLogGC(
-      tablet_servers_, &leader_uuid, &orig_term, &follower_uuid, tserver_behavior));
+      tablet_servers_, &leader_uuid, &orig_term, &follower_uuid,
+      tserver_behavior, delay));
 
   // The catalog manager should evict the replicas which fell behing the WAL
   // segment GC threshold right away.


Mime
View raw message