You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by ad...@apache.org on 2020/01/10 17:51:42 UTC
[kudu] branch master updated: [client] retry to connect to the
cluster when specifing a superset of masters
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 <ch...@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 <ad...@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