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/07 23:01:32 UTC
[kudu-CR] Assign locations to tablet servers and the client in Java
Will Berkeley has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12174
Change subject: Assign locations to tablet servers and the client in Java
......................................................................
Assign locations to tablet servers and the client in Java
Change-Id: I9e2c74ab12f7da187bf6e75d42a3089bc20235db
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.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/TestConnectionCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.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
A java/kudu-client/src/test/resources/assign-location.py
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/KuduTestHarness.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java
9 files changed, 375 insertions(+), 23 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/12174/1
--
To view, visit http://gerrit.cloudera.org:8080/12174
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9e2c74ab12f7da187bf6e75d42a3089bc20235db
Gerrit-Change-Number: 12174
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
[kudu-CR] Assign locations to tablet servers and the client in Java
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12174 )
Change subject: Assign locations to tablet servers and the client in Java
......................................................................
Patch Set 3:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/12174/3/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/12174/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1884
PS3, Line 1884: final String location = tsInfoPB.getLocation();
: return new ServerInfo(uuid, hostPort, inetAddress, location);
> Nit: combine
Done
http://gerrit.cloudera.org:8080/#/c/12174/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/12174/3/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java@49
PS3, Line 49: --
> nit: ' --' or replace with ';'
Done
http://gerrit.cloudera.org:8080/#/c/12174/3/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java@99
PS3, Line 99: public String getLocation() { return location; }
> Nit: I don't think we use this "cram it all on one line" style in Java.
The IDE was visualizing the getters in the file as crammed on one line so I thought that was the style for them :(.
http://gerrit.cloudera.org:8080/#/c/12174/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java:
http://gerrit.cloudera.org:8080/#/c/12174/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@1123
PS3, Line 1123: assertEquals("", client.asyncClient.location);
> Not much of a test though because this assert is true before the listTablet
True, but it still tests that nothing changes by connecting to the master.
http://gerrit.cloudera.org:8080/#/c/12174/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java:
PS3:
> I guess the very first version of this file was had DOS end-of-line symbols
I'll do that in a follow up. I have to redo the whole change if I do it prior to these changes since git calls the entire contents of the file as a conflict.
http://gerrit.cloudera.org:8080/#/c/12174/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java@1
PS3, Line 1: /**
: * Licensed under the Apache License, Version 2.0 (the "License");
: * you may not use this file except in compliance with the License.
: * You may obtain a copy of the License at
: *
: * http://www.apache.org/licenses/LICENSE-2.0
: *
: * Unless required by applicable law or agreed to in writing, software
: * distributed under the License is distributed on an "AS IS" BASIS,
: * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
: * See the License for the specific language governing permissions and
: * limitations under the License. See accompanying LICENSE file.
: */
> nit: while you are at it, maybe unify the license header and make it commen
Done
http://gerrit.cloudera.org:8080/#/c/12174/3/java/kudu-client/src/test/resources/assign-location.py
File java/kudu-client/src/test/resources/assign-location.py:
PS3:
> Can you symlink this to the existing assign-location.py rather than copy it
Seems like it works.
http://gerrit.cloudera.org:8080/#/c/12174/3/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java
File java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java:
http://gerrit.cloudera.org:8080/#/c/12174/3/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java@229
PS3, Line 229: getClass().getResource("/assign-location.py").getFile()
> I'm curious whether this works when tests are run using dist-test.
The precommit ran this test using dist-test, so it does!
http://gerrit.cloudera.org:8080/#/c/12174/3/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java@230
PS3, Line 230: String locationMappingCmdPath = Paths.get(clusterRoot, "/location-assignment.state").toString();
> Nit: too long?
Done
http://gerrit.cloudera.org:8080/#/c/12174/3/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java@633
PS3, Line 633: public MiniKuduClusterBuilder addLocationInfo(Collection<String> locations) {
> Would it be more ergonomic if this added one location at a time, as in addF
Done
--
To view, visit http://gerrit.cloudera.org:8080/12174
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e2c74ab12f7da187bf6e75d42a3089bc20235db
Gerrit-Change-Number: 12174
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 18:58:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] Assign locations to tablet servers and the client in Java
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12174 )
Change subject: Assign locations to tablet servers and the client in Java
......................................................................
Patch Set 3:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/12174/3/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/12174/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1884
PS3, Line 1884: final String location = tsInfoPB.getLocation();
: return new ServerInfo(uuid, hostPort, inetAddress, location);
Nit: combine
http://gerrit.cloudera.org:8080/#/c/12174/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/12174/3/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java@99
PS3, Line 99: public String getLocation() { return location; }
Nit: I don't think we use this "cram it all on one line" style in Java.
http://gerrit.cloudera.org:8080/#/c/12174/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java:
http://gerrit.cloudera.org:8080/#/c/12174/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@1123
PS3, Line 1123: assertEquals("", client.asyncClient.location);
Not much of a test though because this assert is true before the listTabletServers() call too.
http://gerrit.cloudera.org:8080/#/c/12174/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java:
PS3:
What happened to the formatting in this file?
http://gerrit.cloudera.org:8080/#/c/12174/3/java/kudu-client/src/test/resources/assign-location.py
File java/kudu-client/src/test/resources/assign-location.py:
PS3:
Can you symlink this to the existing assign-location.py rather than copy it? Does that work?
http://gerrit.cloudera.org:8080/#/c/12174/3/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java
File java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java:
http://gerrit.cloudera.org:8080/#/c/12174/3/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java@230
PS3, Line 230: String locationMappingCmdPath = Paths.get(clusterRoot, "/location-assignment.state").toString();
Nit: too long?
http://gerrit.cloudera.org:8080/#/c/12174/3/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java@633
PS3, Line 633: public MiniKuduClusterBuilder addLocationInfo(Collection<String> locations) {
Would it be more ergonomic if this added one location at a time, as in addFooFlag? At least the usage for the new annotation would be a little cleaner.
--
To view, visit http://gerrit.cloudera.org:8080/12174
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e2c74ab12f7da187bf6e75d42a3089bc20235db
Gerrit-Change-Number: 12174
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-Comment-Date: Tue, 08 Jan 2019 05:34:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] Assign locations to tablet servers and the client in Java
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/12174
to look at the new patch set (#4).
Change subject: Assign locations to tablet servers and the client in Java
......................................................................
Assign locations to tablet servers and the client in Java
Change-Id: I9e2c74ab12f7da187bf6e75d42a3089bc20235db
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.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/TestConnectionCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.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
A java/kudu-client/src/test/resources/assign-location.py
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/KuduTestHarness.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java
9 files changed, 152 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/12174/4
--
To view, visit http://gerrit.cloudera.org:8080/12174
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9e2c74ab12f7da187bf6e75d42a3089bc20235db
Gerrit-Change-Number: 12174
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] Assign locations to tablet servers and the client in Java
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12174 )
Change subject: Assign locations to tablet servers and the client in Java
......................................................................
Patch Set 5: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12174/5/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java
File java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java:
http://gerrit.cloudera.org:8080/#/c/12174/5/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java@624
PS5, Line 624: * Add one location to the minicluster configuration, consisting of a
Nit: "Adds" to maintain the same tense.
--
To view, visit http://gerrit.cloudera.org:8080/12174
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e2c74ab12f7da187bf6e75d42a3089bc20235db
Gerrit-Change-Number: 12174
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 09:33:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] Assign locations to tablet servers and the client in Java
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12174 )
Change subject: Assign locations to tablet servers and the client in Java
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12174/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/12174/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@331
PS5, Line 331: String location = "";
It looks like tests only need read only access for this. It could also be a useful API for users to be able to get their assigned location. Could this be marked private and a public getter method added?
Similar to other getters this could be exposed, as passthrough, to the KuduClient too.
--
To view, visit http://gerrit.cloudera.org:8080/12174
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e2c74ab12f7da187bf6e75d42a3089bc20235db
Gerrit-Change-Number: 12174
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 16:11:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] Assign locations to tablet servers and the client in Java
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12174 )
Change subject: Assign locations to tablet servers and the client in Java
......................................................................
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12174/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/12174/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@331
PS5, Line 331:
> It looks like tests only need read only access for this. It could also be a
I did not expose the location to users because I did not want to constrain its type to being a string. In the future it might be better to structure it differently. That said, it'd be fine to have a GetLocationString() method.
http://gerrit.cloudera.org:8080/#/c/12174/5/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java
File java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java:
http://gerrit.cloudera.org:8080/#/c/12174/5/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java@624
PS5, Line 624: * Adds one location to the minicluster configuration, consisting of a
> Nit: "Adds" to maintain the same tense.
Done
--
To view, visit http://gerrit.cloudera.org:8080/12174
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e2c74ab12f7da187bf6e75d42a3089bc20235db
Gerrit-Change-Number: 12174
Gerrit-PatchSet: 6
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:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] Assign locations to tablet servers and the client in Java
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/12174
to look at the new patch set (#6).
Change subject: Assign locations to tablet servers and the client in Java
......................................................................
Assign locations to tablet servers and the client in Java
Change-Id: I9e2c74ab12f7da187bf6e75d42a3089bc20235db
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.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/TestConnectionCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.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
A java/kudu-client/src/test/resources/assign-location.py
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/KuduTestHarness.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java
10 files changed, 170 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/12174/6
--
To view, visit http://gerrit.cloudera.org:8080/12174
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9e2c74ab12f7da187bf6e75d42a3089bc20235db
Gerrit-Change-Number: 12174
Gerrit-PatchSet: 6
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] Assign locations to tablet servers and the client in Java
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12174 )
Change subject: Assign locations to tablet servers and the client in Java
......................................................................
Patch Set 7:
Had to resolve a trivial merge conflict.
--
To view, visit http://gerrit.cloudera.org:8080/12174
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e2c74ab12f7da187bf6e75d42a3089bc20235db
Gerrit-Change-Number: 12174
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: Wed, 09 Jan 2019 21:21:47 +0000
Gerrit-HasComments: No
[kudu-CR] Assign locations to tablet servers and the client in Java
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12174 )
Change subject: Assign locations to tablet servers and the client in Java
......................................................................
Patch Set 4: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12174/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java
File java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java:
http://gerrit.cloudera.org:8080/#/c/12174/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java@624
PS4, Line 624: * Adds location info that will configure how locations are assigned to
: * tablet servers and clients. Location strings should be in the form
: * 'location:number'. For example, a location info like
: * ["/L0:2", "/L1:1", "/L2:1"]
: * will assign locations to a total of four clients or tablet servers.
: * 2 tablet servers or clients will get the location '/L0' while one
: * will get location '/L1' and one will get location '/L2'.
This should probably be rewritten to account for the new 'one location at a time' style.
--
To view, visit http://gerrit.cloudera.org:8080/12174
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e2c74ab12f7da187bf6e75d42a3089bc20235db
Gerrit-Change-Number: 12174
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: Tue, 08 Jan 2019 19:06:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] Assign locations to tablet servers and the client in Java
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/12174
to look at the new patch set (#5).
Change subject: Assign locations to tablet servers and the client in Java
......................................................................
Assign locations to tablet servers and the client in Java
Change-Id: I9e2c74ab12f7da187bf6e75d42a3089bc20235db
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.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/TestConnectionCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.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
A java/kudu-client/src/test/resources/assign-location.py
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/KuduTestHarness.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java
9 files changed, 152 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/12174/5
--
To view, visit http://gerrit.cloudera.org:8080/12174
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9e2c74ab12f7da187bf6e75d42a3089bc20235db
Gerrit-Change-Number: 12174
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] Assign locations to tablet servers and the client in Java
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12174 )
Change subject: Assign locations to tablet servers and the client in Java
......................................................................
Assign locations to tablet servers and the client in Java
Change-Id: I9e2c74ab12f7da187bf6e75d42a3089bc20235db
Reviewed-on: http://gerrit.cloudera.org:8080/12174
Tested-by: Kudu Jenkins
Reviewed-by: Grant Henke <gr...@apache.org>
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.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/TestConnectionCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.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
A java/kudu-client/src/test/resources/assign-location.py
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/KuduTestHarness.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java
10 files changed, 170 insertions(+), 37 deletions(-)
Approvals:
Kudu Jenkins: Verified
Grant Henke: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/12174
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I9e2c74ab12f7da187bf6e75d42a3089bc20235db
Gerrit-Change-Number: 12174
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] Assign locations to tablet servers and the client in Java
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/12174
to look at the new patch set (#3).
Change subject: Assign locations to tablet servers and the client in Java
......................................................................
Assign locations to tablet servers and the client in Java
Change-Id: I9e2c74ab12f7da187bf6e75d42a3089bc20235db
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.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/TestConnectionCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.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
A java/kudu-client/src/test/resources/assign-location.py
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/KuduTestHarness.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java
9 files changed, 375 insertions(+), 23 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/12174/3
--
To view, visit http://gerrit.cloudera.org:8080/12174
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9e2c74ab12f7da187bf6e75d42a3089bc20235db
Gerrit-Change-Number: 12174
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)
[kudu-CR] Assign locations to tablet servers and the client in Java
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12174 )
Change subject: Assign locations to tablet servers and the client in Java
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12174
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e2c74ab12f7da187bf6e75d42a3089bc20235db
Gerrit-Change-Number: 12174
Gerrit-PatchSet: 6
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:41:12 +0000
Gerrit-HasComments: No
[kudu-CR] Assign locations to tablet servers and the client in Java
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12174 )
Change subject: Assign locations to tablet servers and the client in Java
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12174/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java
File java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java:
http://gerrit.cloudera.org:8080/#/c/12174/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java@624
PS4, Line 624: * Adds location info that will configure how locations are assigned to
: * tablet servers and clients. Location strings should be in the form
: * 'location:number'. For example, a location info like
: * ["/L0:2", "/L1:1", "/L2:1"]
: * will assign locations to a total of four clients or tablet servers.
: * 2 tablet servers or clients will get the location '/L0' while one
: * will get location '/L1' and one will get location '/L2'.
> This should probably be rewritten to account for the new 'one location at a
Done
--
To view, visit http://gerrit.cloudera.org:8080/12174
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e2c74ab12f7da187bf6e75d42a3089bc20235db
Gerrit-Change-Number: 12174
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:08:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] Assign locations to tablet servers and the client in Java
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12174 )
Change subject: Assign locations to tablet servers and the client in Java
......................................................................
Patch Set 5: Verified+1
Unrelated trace-test failure in TSAN.
--
To view, visit http://gerrit.cloudera.org:8080/12174
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e2c74ab12f7da187bf6e75d42a3089bc20235db
Gerrit-Change-Number: 12174
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:12:19 +0000
Gerrit-HasComments: No
[kudu-CR] Assign locations to tablet servers and the client in Java
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has removed a vote on this change.
Change subject: Assign locations to tablet servers and the client in Java
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/12174
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I9e2c74ab12f7da187bf6e75d42a3089bc20235db
Gerrit-Change-Number: 12174
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] Assign locations to tablet servers and the client in Java
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/12174
to look at the new patch set (#7).
Change subject: Assign locations to tablet servers and the client in Java
......................................................................
Assign locations to tablet servers and the client in Java
Change-Id: I9e2c74ab12f7da187bf6e75d42a3089bc20235db
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.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/TestConnectionCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.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
A java/kudu-client/src/test/resources/assign-location.py
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/KuduTestHarness.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java
10 files changed, 170 insertions(+), 37 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/12174/7
--
To view, visit http://gerrit.cloudera.org:8080/12174
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9e2c74ab12f7da187bf6e75d42a3089bc20235db
Gerrit-Change-Number: 12174
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] Assign locations to tablet servers and the client in Java
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12174 )
Change subject: Assign locations to tablet servers and the client in Java
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12174
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e2c74ab12f7da187bf6e75d42a3089bc20235db
Gerrit-Change-Number: 12174
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:07 +0000
Gerrit-HasComments: No
[kudu-CR] Assign locations to tablet servers and the client in Java
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12174 )
Change subject: Assign locations to tablet servers and the client in Java
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/12174/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/12174/3/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java@49
PS3, Line 49: --
nit: ' --' or replace with ';'
http://gerrit.cloudera.org:8080/#/c/12174/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java:
PS3:
> What happened to the formatting in this file?
I guess the very first version of this file was had DOS end-of-line symbols, and it's still partially true.
Will, while you are at at, maybe run dos2unix for this file?
http://gerrit.cloudera.org:8080/#/c/12174/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java@1
PS3, Line 1: /**
: * Licensed under the Apache License, Version 2.0 (the "License");
: * you may not use this file except in compliance with the License.
: * You may obtain a copy of the License at
: *
: * http://www.apache.org/licenses/LICENSE-2.0
: *
: * Unless required by applicable law or agreed to in writing, software
: * distributed under the License is distributed on an "AS IS" BASIS,
: * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
: * See the License for the specific language governing permissions and
: * limitations under the License. See accompanying LICENSE file.
: */
nit: while you are at it, maybe unify the license header and make it commented by single-line comments?
Also, it seems the text is little bit different from the standard license header, but I'm not sure whether it's crucial to standardize that as well.
http://gerrit.cloudera.org:8080/#/c/12174/3/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java
File java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java:
http://gerrit.cloudera.org:8080/#/c/12174/3/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java@229
PS3, Line 229: getClass().getResource("/assign-location.py").getFile()
I'm curious whether this works when tests are run using dist-test.
--
To view, visit http://gerrit.cloudera.org:8080/12174
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e2c74ab12f7da187bf6e75d42a3089bc20235db
Gerrit-Change-Number: 12174
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-Comment-Date: Tue, 08 Jan 2019 15:22:21 +0000
Gerrit-HasComments: Yes