kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From a...@apache.org
Subject [kudu] branch master updated: [client] retry to connect to the cluster when specifing a superset of masters
Date Fri, 10 Jan 2020 17:51:42 GMT
This is an automated email from the ASF dual-hosted git repository.

adar pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git


The following commit(s) were added to refs/heads/master by this push:
     new 1b945e1  [client] retry to connect to the cluster when specifing a superset of masters
1b945e1 is described below

commit 1b945e16b29bb445932c5e59853ac73e9aba69c1
Author: zhangyifan27 <chinazhangyifan@163.com>
AuthorDate: Mon Jan 6 19:36:56 2020 +0800

    [client] retry to connect to the cluster when specifing a superset of masters
    
    When the user specifies more masters in the connection string than the actual
    existing masters, but couldn't find a leader master, the client should retry
    to connect to the cluster instead of returning with NonRecoverableException.
    This will be useful when we need to add or remove masters.
    
    Change-Id: I66033c3ff6d217ce6b8286c94a7333b90cd26d19
    Reviewed-on: http://gerrit.cloudera.org:8080/14981
    Tested-by: Kudu Jenkins
    Reviewed-by: Adar Dembo <adar@cloudera.com>
---
 .../org/apache/kudu/client/ConnectToCluster.java   |  2 +-
 .../apache/kudu/client/TestConnectToCluster.java   | 71 ++++++++++++++--------
 src/kudu/client/master_rpc.cc                      |  4 +-
 .../integration-tests/master_replication-itest.cc  | 56 ++++++++++-------
 4 files changed, 83 insertions(+), 50 deletions(-)

diff --git a/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java b/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
index 4ab9ad2..917a750 100644
--- a/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
+++ b/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
@@ -253,7 +253,7 @@ final class ConnectToCluster {
 
         List<HostPortPB> knownMastersLocal = knownMasters.get();
         if (knownMastersLocal != null &&
-            knownMastersLocal.size() != numMasters) {
+            knownMastersLocal.size() > numMasters) {
           String msg = String.format(
               "Could not connect to a leader master. " +
               "Client configured with %s master(s) (%s) but cluster indicates it expects
" +
diff --git a/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java
b/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java
index 6cadf5d..256f4a9 100644
--- a/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java
+++ b/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java
@@ -17,6 +17,8 @@
 
 package org.apache.kudu.client;
 
+import java.util.List;
+
 import static org.apache.kudu.consensus.Metadata.RaftPeerPB.Role.FOLLOWER;
 import static org.apache.kudu.consensus.Metadata.RaftPeerPB.Role.LEADER;
 import static org.junit.Assert.assertEquals;
@@ -125,68 +127,76 @@ public class TestConnectToCluster {
 
     // Normal case.
     runTest(
-        makeCTMR(LEADER),
-        makeCTMR(FOLLOWER),
-        makeCTMR(FOLLOWER),
+        makeCTMR(LEADER, MASTERS),
+        makeCTMR(FOLLOWER, MASTERS),
+        makeCTMR(FOLLOWER, MASTERS),
         successResponse);
 
     // Permutation works too.
     runTest(
-        makeCTMR(FOLLOWER),
-        makeCTMR(LEADER),
-        makeCTMR(FOLLOWER),
+        makeCTMR(FOLLOWER, MASTERS),
+        makeCTMR(LEADER, MASTERS),
+        makeCTMR(FOLLOWER, MASTERS),
         successResponse);
 
     // Multiple leaders, that's fine since it might be a TOCTOU situation, or one master
     // is confused. Raft handles this if the client then tries to do something that requires
a
     // replication on the master-side.
     runTest(
-        makeCTMR(LEADER),
-        makeCTMR(LEADER),
-        makeCTMR(FOLLOWER),
+        makeCTMR(LEADER, MASTERS),
+        makeCTMR(LEADER, MASTERS),
+        makeCTMR(FOLLOWER, MASTERS),
         successResponse);
 
     // Mixed bag, still works because there's a leader.
     runTest(
         reusableNRE,
-        makeCTMR(FOLLOWER),
-        makeCTMR(LEADER),
+        makeCTMR(FOLLOWER, MASTERS),
+        makeCTMR(LEADER, MASTERS),
         successResponse);
 
     // All unreachable except one leader, still good.
     runTest(
         reusableNRE,
         reusableNRE,
-        makeCTMR(LEADER),
+        makeCTMR(LEADER, MASTERS),
         successResponse);
 
     // Permutation of the previous.
     runTest(
         reusableNRE,
-        makeCTMR(LEADER),
+        makeCTMR(LEADER, MASTERS),
+        reusableNRE,
+        successResponse);
+
+    // Client try to connect three masters, but the cluster is configure with only one master.
+    // If connect to a leader master, success.
+    runTest(
+        reusableNRE,
         reusableNRE,
+        makeCTMR(LEADER, ImmutableList.of(MASTERS.get(0))),
         successResponse);
 
     // Retry cases.
 
     // Just followers means we retry.
     runTest(
-        makeCTMR(FOLLOWER),
-        makeCTMR(FOLLOWER),
-        makeCTMR(FOLLOWER),
+        makeCTMR(FOLLOWER, MASTERS),
+        makeCTMR(FOLLOWER, MASTERS),
+        makeCTMR(FOLLOWER, MASTERS),
         retryResponse);
 
     // One NRE but we have responsive masters, retry.
     runTest(
-        makeCTMR(FOLLOWER),
-        makeCTMR(FOLLOWER),
+        makeCTMR(FOLLOWER, MASTERS),
+        makeCTMR(FOLLOWER, MASTERS),
         reusableNRE,
         retryResponse);
 
     // One good master but no leader, retry.
     runTest(
         reusableNRE,
-        makeCTMR(FOLLOWER),
+        makeCTMR(FOLLOWER, MASTERS),
         reusableNRE,
         retryResponse);
 
@@ -194,7 +204,7 @@ public class TestConnectToCluster {
     runTest(
         reusableRE,
         reusableNRE,
-        makeCTMR(FOLLOWER),
+        makeCTMR(FOLLOWER, MASTERS),
         retryResponse);
 
     // All recoverable means retry.
@@ -211,6 +221,14 @@ public class TestConnectToCluster {
         reusableNRE,
         retryResponse);
 
+    // Client try to connect three masters, but the cluster is configure with only one master.
+    // If the master hasn't become a leader, retry.
+    runTest(
+        reusableNRE,
+        reusableNRE,
+        makeCTMR(FOLLOWER, ImmutableList.of(MASTERS.get(0))),
+        retryResponse);
+
     // Failure case.
 
     // Can't recover anything, give up.
@@ -265,9 +283,14 @@ public class TestConnectToCluster {
     }
   }
 
-  private static ConnectToMasterResponsePB makeCTMR(Metadata.RaftPeerPB.Role role) {
-    return ConnectToMasterResponsePB.newBuilder()
-        .setRole(role)
-        .build();
+  // Helper method to make a ConnectToMasterResponsePB.
+  private static ConnectToMasterResponsePB makeCTMR(Metadata.RaftPeerPB.Role role,
+                                                    List<HostAndPort> masters) {
+    ConnectToMasterResponsePB.Builder b = ConnectToMasterResponsePB.newBuilder();
+    b.setRole(role);
+    for (HostAndPort master : masters) {
+      b.addMasterAddrs(ProtobufHelper.hostAndPortToPB(master));
+    }
+    return b.build();
   }
 }
diff --git a/src/kudu/client/master_rpc.cc b/src/kudu/client/master_rpc.cc
index 72deb66..f497ead 100644
--- a/src/kudu/client/master_rpc.cc
+++ b/src/kudu/client/master_rpc.cc
@@ -349,9 +349,9 @@ void ConnectToClusterRpc::SingleNodeCallback(int master_idx,
       if (resp.role() != RaftPeerPB::LEADER) {
         string msg;
         if (resp.master_addrs_size() > 0 &&
-            resp.master_addrs_size() != addrs_with_names_.size()) {
+            resp.master_addrs_size() > addrs_with_names_.size()) {
           // If we connected to a non-leader, and it responds that the
-          // number of masters in the cluster don't match the client's
+          // number of masters in the cluster is more than the client's
           // view of the number of masters, then it's likely the client
           // is mis-configured (i.e with a subset of the masters).
           // We'll include that info in the error message.
diff --git a/src/kudu/integration-tests/master_replication-itest.cc b/src/kudu/integration-tests/master_replication-itest.cc
index 5013508..b259e5a 100644
--- a/src/kudu/integration-tests/master_replication-itest.cc
+++ b/src/kudu/integration-tests/master_replication-itest.cc
@@ -111,6 +111,33 @@ class MasterReplicationTest : public KuduTest {
     }
   }
 
+  // Shut the cluster down, start initializing the client, and then
+  // bring the cluster back up during the initialization (but before the
+  // timeout can elapse).
+  Status ConnectToClusterDuringStartup(const vector<string>& master_addrs) {
+    // Shut the cluster down and ...
+    cluster_->Shutdown();
+    // ... start the cluster after a delay.
+    scoped_refptr<kudu::Thread> start_thread;
+    RETURN_NOT_OK(Thread::Create("TestCycleThroughAllMasters", "start_thread",
+                                  &MasterReplicationTest::StartClusterDelayed,
+                                  this,
+                                  1000, // start after 1000 millis.
+                                  &start_thread));
+
+    // The timeouts for both RPCs and operations are increased to cope with slow
+    // clusters (i.e. TSAN builds).
+    shared_ptr<KuduClient> client;
+    KuduClientBuilder builder;
+    builder.master_server_addrs(master_addrs);
+    builder.default_admin_operation_timeout(MonoDelta::FromSeconds(90));
+    builder.default_rpc_timeout(MonoDelta::FromSeconds(15));
+    Status s = builder.Build(&client);
+
+    RETURN_NOT_OK(ThreadJoiner(start_thread.get()).Join());
+    return s;
+  }
+
   Status CreateClient(shared_ptr<KuduClient>* out) {
     KuduClientBuilder builder;
     for (int i = 0; i < cluster_->num_masters(); i++) {
@@ -201,36 +228,19 @@ TEST_F(MasterReplicationTest, TestTimeoutWhenAllMastersAreDown) {
   cluster_.reset();
 }
 
-// Shut the cluster down, start initializing the client, and then
-// bring the cluster back up during the initialization (but before the
-// timeout can elapse).
 TEST_F(MasterReplicationTest, TestCycleThroughAllMasters) {
   vector<string> master_addrs;
   ListMasterServerAddrs(&master_addrs);
 
-  // Shut the cluster down and ...
-  cluster_->Shutdown();
-  // ... start the cluster after a delay.
-  scoped_refptr<kudu::Thread> start_thread;
-  ASSERT_OK(Thread::Create("TestCycleThroughAllMasters", "start_thread",
-                                  &MasterReplicationTest::StartClusterDelayed,
-                                  this,
-                                  1000, // start after 1000 millis.
-                                  &start_thread));
-
   // Verify that the client doesn't give up even though the entire
   // cluster is down for a little while.
-  //
-  // The timeouts for both RPCs and operations are increased to cope with slow
-  // clusters (i.e. TSAN builds).
-  shared_ptr<KuduClient> client;
-  KuduClientBuilder builder;
-  builder.master_server_addrs(master_addrs);
-  builder.default_admin_operation_timeout(MonoDelta::FromSeconds(90));
-  builder.default_rpc_timeout(MonoDelta::FromSeconds(15));
-  EXPECT_OK(builder.Build(&client));
+  EXPECT_OK(ConnectToClusterDuringStartup(master_addrs));
 
-  ASSERT_OK(ThreadJoiner(start_thread.get()).Join());
+  // Verify that if the client was configure with more masters than actual masters
+  // in the cluster, it would also keep retrying to connect to the cluster even though
+  // it couldn't find a leader master for a little while.
+  master_addrs.emplace_back("127.0.0.1:55555");
+  EXPECT_OK(ConnectToClusterDuringStartup(master_addrs));
 }
 
 // Test that every master accepts heartbeats, and that a heartbeat to any


Mime
View raw message