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 2020/04/25 15:08:55 UTC
[kudu] branch master updated: clients: prefer tservers in the same
location when multiple are local
This is an automated email from the ASF dual-hosted git repository.
alexey 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 8d4853f clients: prefer tservers in the same location when multiple are local
8d4853f is described below
commit 8d4853f1525fb189a9ca232544ffd01ac2a39c57
Author: Andrew Wong <aw...@cloudera.com>
AuthorDate: Thu Apr 23 17:40:06 2020 -0700
clients: prefer tservers in the same location when multiple are local
be68ce8 introduced the behavior that mini-servers that are local to a
C++ client are treated as local, even on Linux. As such, a
location-aware mini-cluster would not honor client location awareness,
since all mini-servers would be considered local.
While this surfaced as flakiness of location_awareness-itest, this is an
actual product deficiency, albeit one we don't expect to be very common:
if multiple servers are colocated with a client, and the servers are
assigned different locations, those locations should be honored when
picking tablet servers.
Without this patch, location_assignment-itest is ~24% flaky. With it, it
passed in debug mode 300/300 times.
The same logic is updated in the Java client.
Change-Id: I6a0dbac073ba8a9f80a3f175157a09d06037b0df
Reviewed-on: http://gerrit.cloudera.org:8080/15794
Tested-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
.../java/org/apache/kudu/client/RemoteTablet.java | 21 +++++++---
.../org/apache/kudu/client/TestRemoteTablet.java | 48 ++++++++++++++++------
src/kudu/client/client-internal.cc | 28 +++++++------
.../integration-tests/location_assignment-itest.cc | 10 ++---
4 files changed, 69 insertions(+), 38 deletions(-)
diff --git a/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java b/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
index e330a6e..77c6406 100644
--- a/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
+++ b/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
@@ -189,12 +189,14 @@ public class RemoteTablet implements Comparable<RemoteTablet> {
@Nullable
ServerInfo getClosestServerInfo(String location) {
// This method returns
- // 1. a randomly picked server among local servers, if there is a local server, or
- // 2. a randomly picked server in the same location, if there is a server in the
- // same location, or, finally,
+ // 1. a randomly picked server among local servers, if there is one based
+ // on IP and assigned location, or
+ // 2. a randomly picked server in the same assigned location, if there is a
+ // server in the same location, or, finally,
// 3. a randomly picked server among all tablet servers.
// TODO(wdberkeley): Eventually, the client might use the hierarchical
// structure of a location to determine proximity.
+ // NOTE: this is the same logic implemented in client-internal.cc.
synchronized (tabletServers) {
if (tabletServers.isEmpty()) {
return null;
@@ -205,10 +207,17 @@ public class RemoteTablet implements Comparable<RemoteTablet> {
int randomIndex = randomInt % tabletServers.size();
int index = 0;
for (ServerInfo e : tabletServers.values()) {
- if (e.isLocal()) {
- localServers.add(e);
+ boolean serverInSameLocation = !location.isEmpty() && e.inSameLocation(location);
+
+ // Only consider a server "local" if we're in the same location, or if
+ // there is missing location info.
+ if (location.isEmpty() || e.getLocation().isEmpty() ||
+ serverInSameLocation) {
+ if (e.isLocal()) {
+ localServers.add(e);
+ }
}
- if (e.inSameLocation(location)) {
+ if (serverInSameLocation) {
serversInSameLocation.add(e);
}
if (index == randomIndex) {
diff --git a/java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java b/java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
index f793e41..94aa31b 100644
--- a/java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
+++ b/java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
@@ -105,35 +105,41 @@ public class TestRemoteTablet {
@Test
public void testLocalReplica() {
{
- // Tablet with no replicas in the same location as the client.
+ // Let's examine a tablet where the first UUID is local to the client,
+ // but no UUID is in the same location.
RemoteTablet tablet = getTablet(0, 0, -1);
- // No location for the client.
+ // If the client has no location, we should pick the local server.
assertEquals(kUuids[0], tablet.getClosestServerInfo(kNoLocation).getUuid());
- // Client with location.
- assertEquals(kUuids[0], tablet.getClosestServerInfo(kClientLocation).getUuid());
+ // NOTE: if the client did have a location, because the test replicas are
+ // assigned a different default location, they aren't considered local,
+ // so we would select one at random.
}
{
- // Tablet with a non-local replica in the same location as the client.
+ // Let's examine a tablet where the first UUID is local to the client,
+ // and the second is in the same location.
RemoteTablet tablet = getTablet(0, 0, 1);
- // No location for the client.
+ // If the client has no location, we should pick the local server.
assertEquals(kUuids[0], tablet.getClosestServerInfo(kNoLocation).getUuid());
- // Client with location. The local replica should be chosen.
- assertEquals(kUuids[0], tablet.getClosestServerInfo(kClientLocation).getUuid());
+ // If the client does have a location, we should pick the one in the same
+ // location.
+ assertEquals(kUuids[1], tablet.getClosestServerInfo(kClientLocation).getUuid());
}
{
- // Tablet with a local replica in the same location as the client.
+ // Let's examine a tablet where the first UUID is local to the client and
+ // is also in the same location.
RemoteTablet tablet = getTablet(0, 0, 0);
- // No location for the client.
+ // If the client has no location, we should pick the local server.
assertEquals(kUuids[0], tablet.getClosestServerInfo(kNoLocation).getUuid());
- // Client with location. The local replica should be chosen.
+ // If the client does have a location, we should pick the one in the same
+ // location.
assertEquals(kUuids[0], tablet.getClosestServerInfo(kClientLocation).getUuid());
}
}
@@ -169,13 +175,27 @@ public class TestRemoteTablet {
tablet.getReplicaSelectedServerInfo(ReplicaSelection.LEADER_ONLY, kClientLocation)
.getUuid());
- // CLOSEST_REPLICA picks the local replica even if there's a replica in the same location.
- assertEquals(kUuids[1],
+ // Since there are locations assigned, CLOSEST_REPLICA picks the replica
+ // in the same location, even if there's a local one.
+ assertEquals(kUuids[2],
tablet.getReplicaSelectedServerInfo(ReplicaSelection.CLOSEST_REPLICA, kClientLocation)
.getUuid());
}
{
+ RemoteTablet tablet = getTablet(0, 1, -1);
+
+ // LEADER_ONLY picks the leader even if there's a local replica.
+ assertEquals(kUuids[0],
+ tablet.getReplicaSelectedServerInfo(ReplicaSelection.LEADER_ONLY, kClientLocation)
+ .getUuid());
+
+ // NOTE: the test replicas are assigned a default location. So, even if
+ // there are local replicas, because they are in different locations than
+ // the client, with CLOSEST_REPLICA, a replica is chosen at random.
+ }
+
+ {
RemoteTablet tablet = getTablet(0, -1, 1);
// LEADER_ONLY picks the leader even if there's a replica with the same location.
@@ -232,6 +252,8 @@ public class TestRemoteTablet {
return getTablet(leaderIndex, -1, -1);
}
+ // Returns a three-replica remote tablet that considers the given indices of
+ // replicas to be leader, local to the client, and in the same location.
static RemoteTablet getTablet(int leaderIndex,
int localReplicaIndex,
int sameLocationReplicaIndex) {
diff --git a/src/kudu/client/client-internal.cc b/src/kudu/client/client-internal.cc
index f7bf98c..a0f9550 100644
--- a/src/kudu/client/client-internal.cc
+++ b/src/kudu/client/client-internal.cc
@@ -219,29 +219,33 @@ RemoteTabletServer* KuduClient::Data::SelectTServer(
break;
}
// Choose a replica as follows:
- // 1. If there is a replica local to the client, pick it. If there are
- // multiple, pick a random one.
- // 2. Otherwise, if there is a replica in the same location, pick it. If
- // there are multiple, pick a random one.
+ // 1. If there is a replica local to the client according to its IP and
+ // assigned location, pick it. If there are multiple, pick a random one.
+ // 2. Otherwise, if there is a replica in the same assigned location,
+ // pick it. If there are multiple, pick a random one.
// 3. If there are no local replicas or replicas in the same location,
- // pick a random replica.
+ // pick a random replica.
// TODO(wdberkeley): Eventually, the client might use the hierarchical
// structure of a location to determine proximity.
+ // NOTE: this is the same logic implemented in RemoteTablet.java.
const string client_location = location();
small_vector<RemoteTabletServer*, 1> local;
small_vector<RemoteTabletServer*, 3> same_location;
local.reserve(filtered.size());
same_location.reserve(filtered.size());
for (RemoteTabletServer* rts : filtered) {
- if (IsTabletServerLocal(*rts)) {
- local.push_back(rts);
- }
- if (!client_location.empty()) {
- const string replica_location = rts->location();
- if (client_location == replica_location) {
- same_location.push_back(rts);
+ bool ts_same_location = !client_location.empty() && client_location == rts->location();
+ // Only consider a server "local" if the client is in the same
+ // location, or if there is missing location info.
+ if (client_location.empty() || rts->location().empty() ||
+ ts_same_location) {
+ if (IsTabletServerLocal(*rts)) {
+ local.push_back(rts);
}
}
+ if (ts_same_location) {
+ same_location.push_back(rts);
+ }
}
if (!local.empty()) {
ret = local[rand() % local.size()];
diff --git a/src/kudu/integration-tests/location_assignment-itest.cc b/src/kudu/integration-tests/location_assignment-itest.cc
index 880a276..5a3a95d 100644
--- a/src/kudu/integration-tests/location_assignment-itest.cc
+++ b/src/kudu/integration-tests/location_assignment-itest.cc
@@ -129,12 +129,9 @@ TEST_F(ClientLocationAssignmentITest, Basic) {
"value",
&scans_started));
total_scans_started += scans_started;
-# if defined(__linux__)
- // When running on Linux, the client and tablet servers each have their own
- // IP in the local address space, so no tablet server will be considered
- // local to the client. If there is a tablet server in the same location as
- // the client, it will be the only tablet server scanned. Otherwise, some
- // random tablet server will be scanned.
+ // If there is a tablet server in the same location as the client, it will
+ // be the only tablet server scanned. Otherwise, some random tablet server
+ // will be scanned.
if (!client_colocated_tserver_uuid.empty()) {
if (tserver->uuid() == client_colocated_tserver_uuid) {
ASSERT_EQ(1, scans_started);
@@ -142,7 +139,6 @@ TEST_F(ClientLocationAssignmentITest, Basic) {
ASSERT_EQ(0, scans_started);
}
}
-# endif
}
ASSERT_EQ(1, total_scans_started);
}