[kudu-CR] [java client] Extract RemoteTablet from AsyncKuduClient

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has submitted this change and it was merged.

Change subject: [java client] Extract RemoteTablet from AsyncKuduClient
..


[java client] Extract RemoteTablet from AsyncKuduClient

RemoteTablet is responsible for handling the Java client's view of where 
replicas
are for its tablet, and who the leader is. Extracting this bit of functionality 
is
important if we want to be able to unit test it without bringing up a whole 
client
and possibly a cluster.

This patch makes a minor attempt at simplifying the interfaces, with more work 
to
come. Likewise for unit tests.

Change-Id: I3d06a573e4307c76a7aebab05cd5238fb0d9a2c6
Reviewed-on: http://gerrit.cloudera.org:8080/4719
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert 
---
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/LocatedTablet.java
A java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java
M java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
8 files changed, 345 insertions(+), 318 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/4719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I3d06a573e4307c76a7aebab05cd5238fb0d9a2c6
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [java client] Extract RemoteTablet from AsyncKuduClient

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [java client] Extract RemoteTablet from AsyncKuduClient
..


Patch Set 4: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/4719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d06a573e4307c76a7aebab05cd5238fb0d9a2c6
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java client] Extract RemoteTablet from AsyncKuduClient

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: [java client] Extract RemoteTablet from AsyncKuduClient
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4719/3/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:

Line 55:  * Unlike the C++ client, we don't short-circuit the call to the 
master if it isn't available.
As we discussed, I'm also removing this.


Line 76:   private int leaderIndex = NO_LEADER_INDEX;
> Add @GuardedBy("tabletServers") to this.
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d06a573e4307c76a7aebab05cd5238fb0d9a2c6
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java client] Extract RemoteTablet from AsyncKuduClient

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [java client] Extract RemoteTablet from AsyncKuduClient
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4719/3/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:

Line 76:   private int leaderIndex = NO_LEADER_INDEX;
Add @GuardedBy("tabletServers") to this.


-- 
To view, visit http://gerrit.cloudera.org:8080/4719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d06a573e4307c76a7aebab05cd5238fb0d9a2c6
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java client] Extract RemoteTablet from AsyncKuduClient

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [java client] Extract RemoteTablet from AsyncKuduClient
..


Patch Set 3: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/4719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d06a573e4307c76a7aebab05cd5238fb0d9a2c6
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java client] Extract RemoteTablet from AsyncKuduClient

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: [java client] Extract RemoteTablet from AsyncKuduClient
..


Patch Set 1:

(4 comments)

> (4 comments)
 > 
 > As with the other patch, has anything actually changed (besides
 > method visibility) in the moved code?

As you saw I moved the getting of a TabletClient directly into RemoteTablet, 
and that's about it.

http://gerrit.cloudera.org:8080/#/c/4719/1/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:

PS1, Line 589: tablet == null
> I presume that in the transition from clientFor() to tablet.getLeaderConnec
Yes.

So the tablet can be set to null to "invalidate" the scan, but looking through 
it I don't think it's possible to get here with a null tablet? I'll put an 
assert.


PS1, Line 689: getConnectionToLeader()
> This method doesn't exist.
Oops that got lost in the refactorings.


Line 1304:   rt.init(tabletPb);
> Can we do this as part of createTabletFromPb()? In fact, maybe createTablet
Can't do it in the ctor, it throws an exception.
I like the static RemoteTablet idea.


http://gerrit.cloudera.org:8080/#/c/4719/1/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:

Line 74:   private final ConnectionCache connectionCache;
> Based on our discussion, sounds like this will go away because RemoteTablet
Right, this could go away at some point.


-- 
To view, visit http://gerrit.cloudera.org:8080/4719
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d06a573e4307c76a7aebab05cd5238fb0d9a2c6
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java client] Extract RemoteTablet from AsyncKuduClient

[Jean-Daniel Cryans (Code Review)]

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

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

Change subject: [java client] Extract RemoteTablet from AsyncKuduClient
..

[java client] Extract RemoteTablet from AsyncKuduClient

RemoteTablet is responsible for handling the Java client's view of where 
replicas
are for its tablet, and who the leader is. Extracting this bit of functionality 
is
important if we want to be able to unit test it without bringing up a whole 
client
and possibly a cluster.

This patch makes a minor attempt at simplifying the interfaces, with more work 
to
come. Likewise for unit tests.

Change-Id: I3d06a573e4307c76a7aebab05cd5238fb0d9a2c6
---
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/LocatedTablet.java
A java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java
M java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
8 files changed, 337 insertions(+), 309 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I3d06a573e4307c76a7aebab05cd5238fb0d9a2c6
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans