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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Tue, 08 Jan 2019 18:58:09 +0000 Gerrit-HasComments: Yes
