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 <[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-Comment-Date: Tue, 08 Jan 2019 05:34:26 +0000 Gerrit-HasComments: Yes
