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 <[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:44:47 +0000 Gerrit-HasComments: Yes
