You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org> on 2016/10/25 16:53:06 UTC

[kudu-CR] KUDU-1715. Add a way to set ReplicaSelection to the java client

Jean-Daniel Cryans has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/4837

Change subject: KUDU-1715. Add a way to set ReplicaSelection to the java client
......................................................................

KUDU-1715. Add a way to set ReplicaSelection to the java client

This patch adds a ReplicaSelection option for all the RPCs, which always defaults
to LEADER_ONLY, as well as a way to change it for scanners.

Change-Id: I3bb08c9c78271a0065c8aa8fb9b0f3301f84e828
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
A java/kudu-client/src/main/java/org/apache/kudu/client/ReplicaSelection.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/TestScannerMultiTablet.java
9 files changed, 109 insertions(+), 17 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/4837/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4837
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I3bb08c9c78271a0065c8aa8fb9b0f3301f84e828
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>

[kudu-CR] KUDU-1715. Add a way to set ReplicaSelection to the java client

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-1715. Add a way to set ReplicaSelection to the java client
......................................................................


Patch Set 2:

(5 comments)

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"?


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


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 treat this as a "hint" (give me the leader, or any other replica if one can't be found) or as constraint (throw an exception if a leader can't be found). Given that there is no tserver side enforcement of this I would suggest we treat this as a hint.


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 it choose based on the order sent by the master?


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 default and it's tested pretty much everywhere else, but could you add a simple test all the same? just want to make sure that if we ever change the default we still have test that cover the other case.


-- 
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 <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-HasComments: Yes