Jean-Daniel Cryans has posted comments on this change. Change subject: KUDU-1715. Add a way to set ReplicaSelection to the java client ......................................................................
Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/4837/2/java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java File java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java: PS2, Line 319: us > Nit: use Done http://gerrit.cloudera.org:8080/#/c/4837/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java: PS2, Line 265: ReplicaSelection getReplicaSelection() { : return ReplicaSelection.LEADER_ONLY; : } > why is this hard-coded to "leader only"? We basically make it impossible to configure, unless the actual RPCs (like scanners) allow it. http://gerrit.cloudera.org:8080/#/c/4837/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: PS2, Line 170: us > nit: use Done PS2, Line 171: a UUID for the server that matches the selection, can be null > how can this be null? I think we should put some thought into whether we tr It is a hint, it works the way you expect it. So it can be null if we don't even know where the leader might be, for example. Line 180: return null; > Uh, shouldn't this just throw something? Can throw an unchecked exception yeah. http://gerrit.cloudera.org:8080/#/c/4837/2/java/kudu-client/src/main/java/org/apache/kudu/client/ReplicaSelection.java File java/kudu-client/src/main/java/org/apache/kudu/client/ReplicaSelection.java: PS2, Line 33: random one > is this actually true? are the replicas really selected at random? or does This is lifted straight out of client.h, and as far as the user can tell it will be random (non-deterministic would be a better word given the implementation in the Java client). http://gerrit.cloudera.org:8080/#/c/4837/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestScannerMultiTablet.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestScannerMultiTablet.java: Line 180: public void testLocalScanner() throws Exception { > I'm assuming that you omitted the "leader only" case because it's the defau Sure thing. -- To view, visit http://gerrit.cloudera.org:8080/4837 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3bb08c9c78271a0065c8aa8fb9b0f3301f84e828 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-HasComments: Yes
