You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Will Berkeley (Code Review)" <ge...@cloudera.org> on 2019/01/08 01:06:05 UTC
[kudu-CR] Support location awareness in READ CLOSEST for the Java client
Will Berkeley has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12175
Change subject: Support location awareness in READ_CLOSEST for the Java client
......................................................................
Support location awareness in READ_CLOSEST for the Java client
Change-Id: Ief0f07058cefd0037f4b0f7c60c8b7809dc8313f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ReplicaSelection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestTableLocationsCache.java
8 files changed, 150 insertions(+), 38 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/12175/1
--
To view, visit http://gerrit.cloudera.org:8080/12175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ief0f07058cefd0037f4b0f7c60c8b7809dc8313f
Gerrit-Change-Number: 12175
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
[kudu-CR] Support location awareness in READ CLOSEST for the Java client
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12175
to look at the new patch set (#5).
Change subject: Support location awareness in READ_CLOSEST for the Java client
......................................................................
Support location awareness in READ_CLOSEST for the Java client
Change-Id: Ief0f07058cefd0037f4b0f7c60c8b7809dc8313f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ReplicaSelection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestTableLocationsCache.java
7 files changed, 142 insertions(+), 37 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/12175/5
--
To view, visit http://gerrit.cloudera.org:8080/12175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief0f07058cefd0037f4b0f7c60c8b7809dc8313f
Gerrit-Change-Number: 12175
Gerrit-PatchSet: 5
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] Support location awareness in READ CLOSEST for the Java client
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12175 )
Change subject: Support location awareness in READ_CLOSEST for the Java client
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief0f07058cefd0037f4b0f7c60c8b7809dc8313f
Gerrit-Change-Number: 12175
Gerrit-PatchSet: 7
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 10 Jan 2019 02:12:40 +0000
Gerrit-HasComments: No
[kudu-CR] Support location awareness in READ CLOSEST for the Java client
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12175 )
Change subject: Support location awareness in READ_CLOSEST for the Java client
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/12175/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
http://gerrit.cloudera.org:8080/#/c/12175/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@520
PS2, Line 520: private synchronized String getLocation() {
> I don't think you need this. Access to 'location' doesn't need to be synchr
Done
http://gerrit.cloudera.org:8080/#/c/12175/2/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
File java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java:
http://gerrit.cloudera.org:8080/#/c/12175/2/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java@220
PS2, Line 220: * @param replicaSelection replica selection mechanism to use
> Update the @param list.
Done
http://gerrit.cloudera.org:8080/#/c/12175/2/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java:
http://gerrit.cloudera.org:8080/#/c/12175/2/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java@105
PS2, Line 105: location
> Nit: maybe shorten to 'loc' to avoid the disambiguation below?
Done
http://gerrit.cloudera.org:8080/#/c/12175/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java:
http://gerrit.cloudera.org:8080/#/c/12175/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java@218
PS2, Line 218: static RemoteTablet getTablet(int leaderIndex, int localReplicaIndex, int sameLocationReplicaIndex) {
> Nit: line too long?
Done
--
To view, visit http://gerrit.cloudera.org:8080/12175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief0f07058cefd0037f4b0f7c60c8b7809dc8313f
Gerrit-Change-Number: 12175
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 08 Jan 2019 18:58:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] Support location awareness in READ CLOSEST for the Java client
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12175 )
Change subject: Support location awareness in READ_CLOSEST for the Java client
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/12175/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
http://gerrit.cloudera.org:8080/#/c/12175/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@520
PS2, Line 520: * then do not close the synchronous client.
> Done
I see this message might be related to my review on the other patch. Even though the method doesn't need to be synchronized having a getter instead of a public accessible and mutable variable is still a better api practice.
http://gerrit.cloudera.org:8080/#/c/12175/4/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
File java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java:
http://gerrit.cloudera.org:8080/#/c/12175/4/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java@186
PS4, Line 186: // TODO(KUDU-2348) this doesn't return a random server, but rather returns
Thanks for revamping this message.
http://gerrit.cloudera.org:8080/#/c/12175/4/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java:
http://gerrit.cloudera.org:8080/#/c/12175/4/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java@105
PS4, Line 105: * Returns if the server is in the same location as 'location'.
nit: "Returns true if ..."
--
To view, visit http://gerrit.cloudera.org:8080/12175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief0f07058cefd0037f4b0f7c60c8b7809dc8313f
Gerrit-Change-Number: 12175
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 09 Jan 2019 16:16:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] Support location awareness in READ CLOSEST for the Java client
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12175 )
Change subject: Support location awareness in READ_CLOSEST for the Java client
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12175/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
http://gerrit.cloudera.org:8080/#/c/12175/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@518
PS5, Line 518: *
Nit: got some trailing whitespace here.
http://gerrit.cloudera.org:8080/#/c/12175/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@521
PS5, Line 521: public String getLocationString() {
Not to bike-shed, but why not just 'getLocation'? Isn't that the typical name for an accessor in Java?
--
To view, visit http://gerrit.cloudera.org:8080/12175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief0f07058cefd0037f4b0f7c60c8b7809dc8313f
Gerrit-Change-Number: 12175
Gerrit-PatchSet: 5
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 09 Jan 2019 19:40:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] Support location awareness in READ CLOSEST for the Java client
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12175 )
Change subject: Support location awareness in READ_CLOSEST for the Java client
......................................................................
Support location awareness in READ_CLOSEST for the Java client
Change-Id: Ief0f07058cefd0037f4b0f7c60c8b7809dc8313f
Reviewed-on: http://gerrit.cloudera.org:8080/12175
Tested-by: Kudu Jenkins
Reviewed-by: Grant Henke <gr...@apache.org>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ReplicaSelection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestTableLocationsCache.java
7 files changed, 143 insertions(+), 38 deletions(-)
Approvals:
Kudu Jenkins: Verified
Grant Henke: Looks good to me, approved
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/12175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ief0f07058cefd0037f4b0f7c60c8b7809dc8313f
Gerrit-Change-Number: 12175
Gerrit-PatchSet: 8
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] Support location awareness in READ CLOSEST for the Java client
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12175 )
Change subject: Support location awareness in READ_CLOSEST for the Java client
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12175/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
http://gerrit.cloudera.org:8080/#/c/12175/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@518
PS5, Line 518: *
> Nit: got some trailing whitespace here.
Done
http://gerrit.cloudera.org:8080/#/c/12175/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@521
PS5, Line 521: public String getLocationString() {
> Not to bike-shed, but why not just 'getLocation'? Isn't that the typical na
Because 'location' may not always be a string, but this will always return a string form of 'location'.
--
To view, visit http://gerrit.cloudera.org:8080/12175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief0f07058cefd0037f4b0f7c60c8b7809dc8313f
Gerrit-Change-Number: 12175
Gerrit-PatchSet: 5
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 09 Jan 2019 23:17:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] Support location awareness in READ CLOSEST for the Java client
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12175 )
Change subject: Support location awareness in READ_CLOSEST for the Java client
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/12175/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
http://gerrit.cloudera.org:8080/#/c/12175/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@520
PS2, Line 520: private synchronized String getLocation() {
I don't think you need this. Access to 'location' doesn't need to be synchronized because IIRC it's safe to concurrently access a Java reference provided the object behind it is immutable, and that's the case for 'location', which is only ever set, not mutated. So you could continue to reference 'location' directly, without the accessor.
The 'volatile' keyword provides some extra guarantees but I don't think you need them in this case.
http://gerrit.cloudera.org:8080/#/c/12175/2/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
File java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java:
http://gerrit.cloudera.org:8080/#/c/12175/2/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java@220
PS2, Line 220: * @param replicaSelection replica selection mechanism to use
Update the @param list.
http://gerrit.cloudera.org:8080/#/c/12175/2/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java:
http://gerrit.cloudera.org:8080/#/c/12175/2/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java@105
PS2, Line 105: location
Nit: maybe shorten to 'loc' to avoid the disambiguation below?
http://gerrit.cloudera.org:8080/#/c/12175/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java:
http://gerrit.cloudera.org:8080/#/c/12175/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java@218
PS2, Line 218: static RemoteTablet getTablet(int leaderIndex, int localReplicaIndex, int sameLocationReplicaIndex) {
Nit: line too long?
--
To view, visit http://gerrit.cloudera.org:8080/12175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief0f07058cefd0037f4b0f7c60c8b7809dc8313f
Gerrit-Change-Number: 12175
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 08 Jan 2019 05:44:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] Support location awareness in READ CLOSEST for the Java client
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12175 )
Change subject: Support location awareness in READ_CLOSEST for the Java client
......................................................................
Patch Set 3: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/12175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief0f07058cefd0037f4b0f7c60c8b7809dc8313f
Gerrit-Change-Number: 12175
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 08 Jan 2019 19:06:59 +0000
Gerrit-HasComments: No
[kudu-CR] Support location awareness in READ CLOSEST for the Java client
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12175 )
Change subject: Support location awareness in READ_CLOSEST for the Java client
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12175/3/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java:
http://gerrit.cloudera.org:8080/#/c/12175/3/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java@111
PS3, Line 111: !location.isEmpty() &&
nit: isn't this part redundant once the other two are present?
--
To view, visit http://gerrit.cloudera.org:8080/12175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief0f07058cefd0037f4b0f7c60c8b7809dc8313f
Gerrit-Change-Number: 12175
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 09 Jan 2019 07:04:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] Support location awareness in READ CLOSEST for the Java client
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12175
to look at the new patch set (#4).
Change subject: Support location awareness in READ_CLOSEST for the Java client
......................................................................
Support location awareness in READ_CLOSEST for the Java client
Change-Id: Ief0f07058cefd0037f4b0f7c60c8b7809dc8313f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ReplicaSelection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestTableLocationsCache.java
7 files changed, 140 insertions(+), 37 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/12175/4
--
To view, visit http://gerrit.cloudera.org:8080/12175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief0f07058cefd0037f4b0f7c60c8b7809dc8313f
Gerrit-Change-Number: 12175
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] Support location awareness in READ CLOSEST for the Java client
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12175 )
Change subject: Support location awareness in READ_CLOSEST for the Java client
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12175/3/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java:
http://gerrit.cloudera.org:8080/#/c/12175/3/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java@111
PS3, Line 111: !location.isEmpty() &&
> nit: isn't this part redundant once the other two are present?
Done
--
To view, visit http://gerrit.cloudera.org:8080/12175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief0f07058cefd0037f4b0f7c60c8b7809dc8313f
Gerrit-Change-Number: 12175
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 09 Jan 2019 09:08:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] Support location awareness in READ CLOSEST for the Java client
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12175
to look at the new patch set (#7).
Change subject: Support location awareness in READ_CLOSEST for the Java client
......................................................................
Support location awareness in READ_CLOSEST for the Java client
Change-Id: Ief0f07058cefd0037f4b0f7c60c8b7809dc8313f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ReplicaSelection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestTableLocationsCache.java
7 files changed, 143 insertions(+), 38 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/12175/7
--
To view, visit http://gerrit.cloudera.org:8080/12175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief0f07058cefd0037f4b0f7c60c8b7809dc8313f
Gerrit-Change-Number: 12175
Gerrit-PatchSet: 7
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] Support location awareness in READ CLOSEST for the Java client
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12175 )
Change subject: Support location awareness in READ_CLOSEST for the Java client
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief0f07058cefd0037f4b0f7c60c8b7809dc8313f
Gerrit-Change-Number: 12175
Gerrit-PatchSet: 7
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 10 Jan 2019 02:31:15 +0000
Gerrit-HasComments: No
[kudu-CR] Support location awareness in READ CLOSEST for the Java client
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12175 )
Change subject: Support location awareness in READ_CLOSEST for the Java client
......................................................................
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/12175/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
http://gerrit.cloudera.org:8080/#/c/12175/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@520
PS2, Line 520: */
> I see this message might be related to my review on the other patch. Even t
Done
http://gerrit.cloudera.org:8080/#/c/12175/4/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
File java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java:
http://gerrit.cloudera.org:8080/#/c/12175/4/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java@186
PS4, Line 186: // TODO(KUDU-2348) this doesn't return a random server, but rather returns
> Thanks for revamping this message.
I am a star among friends.
http://gerrit.cloudera.org:8080/#/c/12175/4/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java:
http://gerrit.cloudera.org:8080/#/c/12175/4/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java@105
PS4, Line 105: * Returns true if the server is in the same location as 'location'.
> nit: "Returns true if ..."
Done
--
To view, visit http://gerrit.cloudera.org:8080/12175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief0f07058cefd0037f4b0f7c60c8b7809dc8313f
Gerrit-Change-Number: 12175
Gerrit-PatchSet: 5
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 09 Jan 2019 19:35:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] Support location awareness in READ CLOSEST for the Java client
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12175
to look at the new patch set (#2).
Change subject: Support location awareness in READ_CLOSEST for the Java client
......................................................................
Support location awareness in READ_CLOSEST for the Java client
Change-Id: Ief0f07058cefd0037f4b0f7c60c8b7809dc8313f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ReplicaSelection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestTableLocationsCache.java
7 files changed, 149 insertions(+), 37 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/12175/2
--
To view, visit http://gerrit.cloudera.org:8080/12175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief0f07058cefd0037f4b0f7c60c8b7809dc8313f
Gerrit-Change-Number: 12175
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] Support location awareness in READ CLOSEST for the Java client
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12175 )
Change subject: Support location awareness in READ_CLOSEST for the Java client
......................................................................
Patch Set 4: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/12175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief0f07058cefd0037f4b0f7c60c8b7809dc8313f
Gerrit-Change-Number: 12175
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 09 Jan 2019 09:32:34 +0000
Gerrit-HasComments: No
[kudu-CR] Support location awareness in READ CLOSEST for the Java client
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12175
to look at the new patch set (#3).
Change subject: Support location awareness in READ_CLOSEST for the Java client
......................................................................
Support location awareness in READ_CLOSEST for the Java client
Change-Id: Ief0f07058cefd0037f4b0f7c60c8b7809dc8313f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ReplicaSelection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestTableLocationsCache.java
7 files changed, 141 insertions(+), 37 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/12175/3
--
To view, visit http://gerrit.cloudera.org:8080/12175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief0f07058cefd0037f4b0f7c60c8b7809dc8313f
Gerrit-Change-Number: 12175
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>