You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2020/04/24 00:46:22 UTC
[kudu-CR] client: prefer local tservers in the same location
Hello Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/15794
to review the following change.
Change subject: client: prefer local tservers in the same location
......................................................................
client: prefer local tservers in the same location
be68ce8 introduced the behavior that mini servers that are local to a
client are treated as local, even on Linux. As such, a location-aware
mini-cluster would not honor client location awareness, since all
servers would be considered local.
While this surfaced as flakiness of location_awareness-itest, this is an
actual product deficiency: if two servers are colocated, but are
assigned different locations, those locations should be honored when
picking tablet servers.
Without this patch, location_assignment-itest is ~24% flaky. With it,
this passed in debug mode 300/300 times.
Change-Id: I6a0dbac073ba8a9f80a3f175157a09d06037b0df
---
M src/kudu/client/client-internal.cc
M src/kudu/integration-tests/location_assignment-itest.cc
2 files changed, 14 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/15794/1
--
To view, visit http://gerrit.cloudera.org:8080/15794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6a0dbac073ba8a9f80a3f175157a09d06037b0df
Gerrit-Change-Number: 15794
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
[kudu-CR] clients: prefer tservers in the same location when multiple are local
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15794 )
Change subject: clients: prefer tservers in the same location when multiple are local
......................................................................
Patch Set 4: Verified+1
Unrelated test failure:
testBatchErrorCauseSessionStuck(org.apache.kudu.client.TestAsyncKuduSession)
java.lang.AssertionError
at org.junit.Assert.fail(Assert.java:87)
at org.junit.Assert.assertTrue(Assert.java:42)
at org.junit.Assert.assertFalse(Assert.java:65)
at org.junit.Assert.assertFalse(Assert.java:75)
at org.apache.kudu.client.TestAsyncKuduSession.testBatchErrorCauseSessionStuck(TestAsyncKuduSession.java:124)
--
To view, visit http://gerrit.cloudera.org:8080/15794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a0dbac073ba8a9f80a3f175157a09d06037b0df
Gerrit-Change-Number: 15794
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 25 Apr 2020 15:04:14 +0000
Gerrit-HasComments: No
[kudu-CR] client: prefer tservers in the same location when multiple are local
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15794 )
Change subject: client: prefer tservers in the same location when multiple are local
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15794/2/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:
http://gerrit.cloudera.org:8080/#/c/15794/2/src/kudu/client/client-internal.cc@222
PS2, Line 222: // 1. If there is a replica local to the client according to its IP and
: // location, 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.
> I don't understand the difference between local and same location?
Location refers to a location assigned by the location-assigning script. "Local" in this context refers to the output of IsTabletServerLocal(), which looks at some host ports https://github.com/apache/kudu/blob/master/src/kudu/client/client-internal.cc#L445
--
To view, visit http://gerrit.cloudera.org:8080/15794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a0dbac073ba8a9f80a3f175157a09d06037b0df
Gerrit-Change-Number: 15794
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 24 Apr 2020 02:00:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] clients: prefer tservers in the same location when multiple are local
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15794 )
Change subject: clients: prefer tservers in the same location when multiple are local
......................................................................
Patch Set 3:
Thank you for fixing this!
It seem the test failure is relevant:
There were 2 failures:
1) testLocalReplica(org.apache.kudu.client.TestRemoteTablet)
org.junit.ComparisonFailure: expected:<uuid-[0]> but was:<uuid-[1]>
at org.junit.Assert.assertEquals(Assert.java:117)
at org.junit.Assert.assertEquals(Assert.java:146)
at org.apache.kudu.client.TestRemoteTablet.testLocalReplica(TestRemoteTablet.java:126)
2) testReplicaSelection(org.apache.kudu.client.TestRemoteTablet)
org.junit.ComparisonFailure: expected:<uuid-[1]> but was:<uuid-[2]>
at org.junit.Assert.assertEquals(Assert.java:117)
at org.junit.Assert.assertEquals(Assert.java:146)
at org.apache.kudu.client.TestRemoteTablet.testReplicaSelection(TestRemoteTablet.java:173)
--
To view, visit http://gerrit.cloudera.org:8080/15794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a0dbac073ba8a9f80a3f175157a09d06037b0df
Gerrit-Change-Number: 15794
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 24 Apr 2020 23:25:16 +0000
Gerrit-HasComments: No
[kudu-CR] clients: prefer tservers in the same location when multiple are local
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15794 )
Change subject: clients: prefer tservers in the same location when multiple are local
......................................................................
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>
---
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
M src/kudu/client/client-internal.cc
M src/kudu/integration-tests/location_assignment-itest.cc
4 files changed, 69 insertions(+), 38 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/15794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I6a0dbac073ba8a9f80a3f175157a09d06037b0df
Gerrit-Change-Number: 15794
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] client: prefer tservers in the same location when multiple are local
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15794 )
Change subject: client: prefer tservers in the same location when multiple are local
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15794/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15794/2//COMMIT_MSG@14
PS2, Line 14: this is an
: actual product deficiency: if two servers are colocated, but are
: assigned different locations, those locations should be honored when
: picking tablet servers
> +1
I was just asking to add a note that the bug manifested itself only in some artificial and test scenarios. If the consensus that it doesn't make sense to add such a comment, I'm OK with that.
--
To view, visit http://gerrit.cloudera.org:8080/15794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a0dbac073ba8a9f80a3f175157a09d06037b0df
Gerrit-Change-Number: 15794
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 24 Apr 2020 17:07:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] client: prefer tservers in the same location when multiple are local
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/15794 )
Change subject: client: prefer tservers in the same location when multiple are local
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15794/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15794/2//COMMIT_MSG@14
PS2, Line 14: this is an
: actual product deficiency: if two servers are colocated, but are
: assigned different locations, those locations should be honored when
: picking tablet servers
> I agree that this looks like a deficiency, but it's artificial and could be
Though it would be considered bad practice, I do think it's worth handling well. I suspect a lot of test/dev clusters will have this same setup.
http://gerrit.cloudera.org:8080/#/c/15794/2/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:
http://gerrit.cloudera.org:8080/#/c/15794/2/src/kudu/client/client-internal.cc@237
PS2, Line 237: if (client_location.empty() || ts_same_location) {
Is there a Java client equivalent of this logic that needs to be updated?
--
To view, visit http://gerrit.cloudera.org:8080/15794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a0dbac073ba8a9f80a3f175157a09d06037b0df
Gerrit-Change-Number: 15794
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 24 Apr 2020 15:22:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] clients: prefer tservers in the same location when multiple are local
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15794 )
Change subject: clients: prefer tservers in the same location when multiple are local
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a0dbac073ba8a9f80a3f175157a09d06037b0df
Gerrit-Change-Number: 15794
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 25 Apr 2020 15:04:18 +0000
Gerrit-HasComments: No
[kudu-CR] clients: prefer tservers in the same location when multiple are local
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15794 )
Change subject: clients: prefer tservers in the same location when multiple are local
......................................................................
Patch Set 4:
Thank you for the fix, Andrew!
--
To view, visit http://gerrit.cloudera.org:8080/15794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a0dbac073ba8a9f80a3f175157a09d06037b0df
Gerrit-Change-Number: 15794
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 25 Apr 2020 15:04:37 +0000
Gerrit-HasComments: No
[kudu-CR] client: prefer tservers in the same location when multiple are local
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15794 )
Change subject: client: prefer tservers in the same location when multiple are local
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15794/2/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:
http://gerrit.cloudera.org:8080/#/c/15794/2/src/kudu/client/client-internal.cc@222
PS2, Line 222: // 1. If there is a replica local to the client according to its IP and
: // location, 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.
> Location refers to a location assigned by the location-assigning script. "L
In this context, location refers to a failure domain, like a rack.
--
To view, visit http://gerrit.cloudera.org:8080/15794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a0dbac073ba8a9f80a3f175157a09d06037b0df
Gerrit-Change-Number: 15794
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 24 Apr 2020 02:06:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] client: prefer tservers in the same location when multiple are local
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15794 )
Change subject: client: prefer tservers in the same location when multiple are local
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15794/2/src/kudu/integration-tests/location_assignment-itest.cc
File src/kudu/integration-tests/location_assignment-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15794/2/src/kudu/integration-tests/location_assignment-itest.cc@a132
PS2, Line 132:
I ran this on macOS and it passes.
--
To view, visit http://gerrit.cloudera.org:8080/15794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a0dbac073ba8a9f80a3f175157a09d06037b0df
Gerrit-Change-Number: 15794
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 24 Apr 2020 01:00:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] client: prefer tservers in the same location when multiple are local
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15794
to look at the new patch set (#2).
Change subject: client: prefer tservers in the same location when multiple are local
......................................................................
client: prefer tservers in the same location when multiple are local
be68ce8 introduced the behavior that mini servers that are local to a
client are treated as local, even on Linux. As such, a location-aware
mini-cluster would not honor client location awareness, since all
servers would be considered local.
While this surfaced as flakiness of location_awareness-itest, this is an
actual product deficiency: if two servers are colocated, but are
assigned different locations, those locations should be honored when
picking tablet servers.
Without this patch, location_assignment-itest is ~24% flaky. With it,
this passed in debug mode 300/300 times.
Change-Id: I6a0dbac073ba8a9f80a3f175157a09d06037b0df
---
M src/kudu/client/client-internal.cc
M src/kudu/integration-tests/location_assignment-itest.cc
2 files changed, 14 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/15794/2
--
To view, visit http://gerrit.cloudera.org:8080/15794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6a0dbac073ba8a9f80a3f175157a09d06037b0df
Gerrit-Change-Number: 15794
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] client: prefer tservers in the same location when multiple are local
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15794 )
Change subject: client: prefer tservers in the same location when multiple are local
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a0dbac073ba8a9f80a3f175157a09d06037b0df
Gerrit-Change-Number: 15794
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 24 Apr 2020 16:49:19 +0000
Gerrit-HasComments: No
[kudu-CR] client: prefer tservers in the same location when multiple are local
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15794 )
Change subject: client: prefer tservers in the same location when multiple are local
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15794/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15794/2//COMMIT_MSG@14
PS2, Line 14: this is an
: actual product deficiency: if two servers are colocated, but are
: assigned different locations, those locations should be honored when
: picking tablet servers
> Though it would be considered bad practice, I do think it's worth handling
+1
I agree it's worth handling well since test clusters use same network interface for different servers.
http://gerrit.cloudera.org:8080/#/c/15794/2/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:
http://gerrit.cloudera.org:8080/#/c/15794/2/src/kudu/client/client-internal.cc@237
PS2, Line 237: if (client_location.empty() || ts_same_location) {
> Is there a Java client equivalent of this logic that needs to be updated?
There is logic in getClosestServerInfo(): https://github.com/apache/kudu/blob/1d1a85804b8ce132021661a8fdb053141c2781c2/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java#L179-L229
--
To view, visit http://gerrit.cloudera.org:8080/15794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a0dbac073ba8a9f80a3f175157a09d06037b0df
Gerrit-Change-Number: 15794
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 24 Apr 2020 16:49:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] client: prefer tservers in the same location when multiple are local
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15794 )
Change subject: client: prefer tservers in the same location when multiple are local
......................................................................
Patch Set 2: Code-Review+1
(2 comments)
Thank you for fixing the flake. I didn't get to looking at this.
http://gerrit.cloudera.org:8080/#/c/15794/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15794/2//COMMIT_MSG@14
PS2, Line 14: this is an
: actual product deficiency: if two servers are colocated, but are
: assigned different locations, those locations should be honored when
: picking tablet servers
I agree that this looks like a deficiency, but it's artificial and could be seen in only non-practical or misconfiguration case.
If two collocated servers are in different location, then that's sort of a nonsense setup. This is because if that single node fails, servers in two locations become unavailable, which defies the whole idea of using location awareness to achieve fault tolerance.
Maybe, it's worth adding a blurb about that?
http://gerrit.cloudera.org:8080/#/c/15794/2/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:
http://gerrit.cloudera.org:8080/#/c/15794/2/src/kudu/client/client-internal.cc@222
PS2, Line 222: // 1. If there is a replica local to the client according to its IP and
: // location, 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.
> I don't understand the difference between local and same location?
This might be useful to understand what location awareness means in Kudu: https://kudu.apache.org/docs/administration.html#rack_awareness
There is also a blogpost about this feature: https://kudu.apache.org/2019/04/30/location-awareness.html
--
To view, visit http://gerrit.cloudera.org:8080/15794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a0dbac073ba8a9f80a3f175157a09d06037b0df
Gerrit-Change-Number: 15794
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 24 Apr 2020 07:25:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] clients: prefer tservers in the same location when multiple are local
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Grant Henke, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15794
to look at the new patch set (#4).
Change subject: clients: prefer tservers in the same location when multiple are local
......................................................................
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
---
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
M src/kudu/client/client-internal.cc
M src/kudu/integration-tests/location_assignment-itest.cc
4 files changed, 69 insertions(+), 38 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/15794/4
--
To view, visit http://gerrit.cloudera.org:8080/15794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6a0dbac073ba8a9f80a3f175157a09d06037b0df
Gerrit-Change-Number: 15794
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] clients: prefer tservers in the same location when multiple are local
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: clients: prefer tservers in the same location when multiple are local
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/15794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I6a0dbac073ba8a9f80a3f175157a09d06037b0df
Gerrit-Change-Number: 15794
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] clients: prefer tservers in the same location when multiple are local
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15794 )
Change subject: clients: prefer tservers in the same location when multiple are local
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15794/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15794/2//COMMIT_MSG@14
PS2, Line 14: 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 loc
> I was just asking to add a note that the bug manifested itself only in some
Done
http://gerrit.cloudera.org:8080/#/c/15794/2/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:
http://gerrit.cloudera.org:8080/#/c/15794/2/src/kudu/client/client-internal.cc@237
PS2, Line 237: if (client_location.empty() || ts_same_location) {
> There is logic in getClosestServerInfo(): https://github.com/apache/kudu/bl
Done
--
To view, visit http://gerrit.cloudera.org:8080/15794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a0dbac073ba8a9f80a3f175157a09d06037b0df
Gerrit-Change-Number: 15794
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 24 Apr 2020 20:09:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] client: prefer tservers in the same location when multiple are local
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15794 )
Change subject: client: prefer tservers in the same location when multiple are local
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15794/2/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:
http://gerrit.cloudera.org:8080/#/c/15794/2/src/kudu/client/client-internal.cc@222
PS2, Line 222: // 1. If there is a replica local to the client according to its IP and
: // location, 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.
I don't understand the difference between local and same location?
Same location of tablet server wrt client, right?
--
To view, visit http://gerrit.cloudera.org:8080/15794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a0dbac073ba8a9f80a3f175157a09d06037b0df
Gerrit-Change-Number: 15794
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 24 Apr 2020 01:54:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] clients: prefer tservers in the same location when multiple are local
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Grant Henke, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15794
to look at the new patch set (#3).
Change subject: clients: prefer tservers in the same location when multiple are local
......................................................................
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
---
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
M src/kudu/client/client-internal.cc
M src/kudu/integration-tests/location_assignment-itest.cc
3 files changed, 25 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/15794/3
--
To view, visit http://gerrit.cloudera.org:8080/15794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6a0dbac073ba8a9f80a3f175157a09d06037b0df
Gerrit-Change-Number: 15794
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)