[kudu-CR] [java-client] separating Connection

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [java-client] separating Connection
..


Patch Set 13:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/7146/13//COMMIT_MSG
Commit Message:

Line 11: The TabletClient has been renamed into RpcHelper.  Also,
update 'RpcHelper'


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

Line 151:   /**
> Just a collection of object representing connections to the servers, no any
Hmm so I don't think the new comment is adding much clarity.  I think i'd 
prefer the original, minus the word 'trivial'.  My question about 'trivial' is 
that it suggests the cache is deficient or not-fully feature in some way, but 
AFAICT it's exactly what we need :)


Line 170: 
> IntellyJIdea was pointing at this as a broken link.
This is relative to where you have set up the root IntelliJ project.  I'd 
prefer it you leave it as is, since it's also possible to configure IntelliJ 
relative to the repo root.


http://gerrit.cloudera.org:8080/#/c/7146/10/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
File java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java:

Line 74:  * a response is currently awaited, as well as temporarily buffered 
RPCs that are waiting
> It seems this comment is stale: there is no need to care about Netty channe
Great!


Line 598: 
> Done -- I ended up calling it 'READY'.
Good name.


http://gerrit.cloudera.org:8080/#/c/7146/13/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
File java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java:

Line 479:* @return true iff the connection is in the READY state.
omit the trailing period


http://gerrit.cloudera.org:8080/#/c/7146/13/java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java
File java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java:

Line 51:  * @{link Connection} objects.
I think the @ needs to go inside the opening brace


Line 61:   /** The logger to use. */
This comment can be omitted, it's obvious from context


Line 73:* @param client top-level Kudu client object.
omit trailing periods on param docs


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [java-client] separating Connection

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new patch set (#13).

Change subject: [java-client] separating Connection
..

[java-client] separating Connection

This patch separates lower-level, connection-related functionality
from the TabletClient class into the new Connection class.
The TabletClient has been renamed into RpcHelper.  Also,
this patch contains other micro-updates on the related code.

In addition, this patch addresses KUDU-1878.

This work is done in the context of KUDU-2013.

Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
A java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.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/Negotiator.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/RpcProxy.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
D java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITFaultTolerantScanner.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITNonFaultTolerantScanner.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.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/TestTimeouts.java
21 files changed, 1,395 insertions(+), 1,266 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/7146/13
-- 
To view, visit http://gerrit.cloudera.org:8080/7146
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [java-client] separating Connection

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

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

to look at the new patch set (#12).

Change subject: [java-client] separating Connection
..

[java-client] separating Connection

This patch separates lower-level, connection-related functionality
from the TabletClient class into the new Connection class.
The TabletClient has been renamed into RpcHelper.  Also,
this patch contains other micro-updates on the related code.

In addition, this patch addresses KUDU-1878.

This work is done in the context of KUDU-2013.

Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
A java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.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/Negotiator.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/RpcProxy.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
D java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITFaultTolerantScanner.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITNonFaultTolerantScanner.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.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/TestTimeouts.java
21 files changed, 1,385 insertions(+), 1,266 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/7146/12
-- 
To view, visit http://gerrit.cloudera.org:8080/7146
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [java-client] separating Connection

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [java-client] separating Connection
..


Patch Set 10:

(20 comments)

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

Line 603
> Would this be difficult to keep?  It would be better not to remove it since
Right, I returned it back already.  No, it's not a problem to keep it.


Line 151:   /** A trivial cache to keep track of already opened connections to 
Kudu servers. */
> What does trivial mean in this context?
Just a collection of object representing connections to the servers, no any 
special behavior.  I'll update the comment.


Line 170:* @see ../src/kudu/common/common.proto
> Why this change?
IntellyJIdea was pointing at this as a broken link.


Line 238: 
> Add a method doc.
Done


Line 754: closeRequest.attempt++;
> What's the significance of the ArrayList in the return type?  Could it just
Nothing particular, just propagated the result from disconnectEverything().  
Yes, it can be just Deferred.

Also, this method is used only in tests, so I'll just remove it and replace 
with { getConnection(), disconnectEverything() } combination.


Line 934:* the provided callback will be called.
> Wrap the second arg
Done


http://gerrit.cloudera.org:8080/#/c/7146/10/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
File java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java:

PS10, Line 70: RPC
> RPCs
Done


PS10, Line 71: awaiting
> waiting
Done


Line 74:  * This class needs careful synchronization. It's a non-sharable 
handler,
> Could you make this paragraph more precise by calling out that 'everything 
It seems this comment is stale: there is no need to care about Netty channel 
anymore since it's managed internally.  Also, synchronization is already taken 
care of in other places, and I fixed the issue with the disconnect() method.


Line 84:  */
> yea I consider the use of socket timeouts to be a bug rather than a feature
ok, I added TODO


Line 246:   ArrayList queued;
> This can be a List
Done


Line 254: queued = queuedMessages;
> The declaration of 'queued' can be moved here.
Done


Line 320: "Tablet server sent error " + error.getMessage();
> This should probably be 'Server sent error'
Done


Line 536: ArrayList queued;
> This can be a List
Done


Line 537: HashMap> inflight;
> This can be a Map<..>
Done


Line 568: inflight.clear();
> Is this necessary?
Good catch -- no, it's not necessary.  It seems I just re-did the original 
implementation of the loop where the elements were removed from the container 
while iterating over them.


Line 598: NEGOTIATED,   // The connection has negotiated and ready to use.
> I think this state would be clearer if it was called 'Open' or 'Running' or
Done -- I ended up calling it 'READY'.


http://gerrit.cloudera.org:8080/#/c/7146/10/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java:

PS10, Line 36:  Kudu server-side components
> I think it was clearer as 'masters and tablet servers'
Done


http://gerrit.cloudera.org:8080/#/c/7146/10/java/kudu-client/src/main/java/org/apache/kudu/client/RpcHelper.java
File java/kudu-client/src/main/java/org/apache/kudu/client/RpcHelper.java:

Line 49:  * This is a stateless helper to send RPCs to a Kudu server.
> It doesn't appear to be stateless, although it could easily be made so.  Pe
It's stateless in the sense that it does not store any state in it, just 
references to the AsyncKuduClient and Connection instances.

There would be more methods than one if making it static, I think.  I looked at 
that approach and in some cases it would require passing both connection and 
Kudu client reference, like in ConnectToCluster.java.  That's why I decided to 
keep it like this.


Line 57: public class RpcHelper {
> Is this the same as the Proxy on the c++ side?
Not exactly, but close.

Todd was not happy with the name for this class, so I'll rename it into 
RpcProxy.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [java-client] separating Connection

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

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

to look at the new patch set (#11).

Change subject: [java-client] separating Connection
..

[java-client] separating Connection

This patch separates lower-level, connection-related functionality
from the TabletClient class into the new Connection class.
The TabletClient has been renamed into RpcHelper.  Also,
this patch contains other micro-updates on the related code.

In addition, this patch addresses KUDU-1878.

This work is done in the context of KUDU-2013.

Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
A java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.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/Negotiator.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/RpcHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
D java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITFaultTolerantScanner.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITNonFaultTolerantScanner.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.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/TestTimeouts.java
21 files changed, 1,381 insertions(+), 1,266 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/7146/11
-- 
To view, visit http://gerrit.cloudera.org:8080/7146
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [java-client] separating Connection

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: [java-client] separating Connection
..


Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7146/10/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
File java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java:

Line 84:  */
> yea I consider the use of socket timeouts to be a bug rather than a feature
Not aware of a jira.

The ReadTimeoutHandler was the only thing I found in Netty that was a proxy for 
a socket timeout. Of course it's very coarse grained and Todd hates it.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [java-client] separating Connection

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [java-client] separating Connection
..


Patch Set 10:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7146/10/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
File java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java:

Line 84:  */
> Could you add a comment to the class explaining why it's using a socket rea
yea I consider the use of socket timeouts to be a bug rather than a feature... 
timeouts should be call-scoped rather than something on the socket level. but I 
dont think it's within scope of this patch to fix that. maybe a TODO? I thought 
we had a JIRA about this but I can't find it now - maybe JD knows.


http://gerrit.cloudera.org:8080/#/c/7146/9/java/kudu-client/src/main/java/org/apache/kudu/client/RpcHelper.java
File java/kudu-client/src/main/java/org/apache/kudu/client/RpcHelper.java:

PS9, Line 57: RpcHelper
nit: I have a bit of a pet peeve about classes named "Helper" because it's not 
very illustrative about what the class actually does. Maybe this is 
'OutboundRpc' or 'RpcRouter' or 'RpcRetrier' or 'InFlightRpc' or something like 
that? (still have to review this patch in whole so maybe I'll have a better 
idea for a name after doing so!)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [java-client] separating Connection

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [java-client] separating Connection
..


Patch Set 10:

(20 comments)

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

Line 603
Would this be difficult to keep?  It would be better not to remove it since 
it's in the public API (even thought this whole class is unstable).


Line 151:   /** A trivial cache to keep track of already opened connections to 
Kudu servers. */
What does trivial mean in this context?


Line 170:* @see ../src/kudu/common/common.proto
Why this change?


Line 238: 
Add a method doc.


Line 754: closeRequest.attempt++;
What's the significance of the ArrayList in the return type?  Could it just be 
Deferred ?


Line 934:* the provided callback will be called.
Wrap the second arg


http://gerrit.cloudera.org:8080/#/c/7146/10/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
File java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java:

PS10, Line 70: RPC
RPCs


PS10, Line 71: awaiting
waiting


Line 74:  * This class needs careful synchronization. It's a non-sharable 
handler,
Could you make this paragraph more precise by calling out that 'everything 
else' is marked with @GuardedBy annotations.


Line 84:  */
Could you add a comment to the class explaining why it's using a socket read 
timeout?  That detail is different than C++, and I frequently can't remember 
why it differs.


Line 246:   ArrayList queued;
This can be a List


Line 254: queued = queuedMessages;
The declaration of 'queued' can be moved here.


Line 320: "Tablet server sent error " + error.getMessage();
This should probably be 'Server sent error'


Line 536: ArrayList queued;
This can be a List


Line 537: HashMap> inflight;
This can be a Map<..>


Line 568: inflight.clear();
Is this necessary?


Line 598: NEGOTIATED,   // The connection has negotiated and ready to use.
I think this state would be clearer if it was called 'Open' or 'Running' or 
something like that.


http://gerrit.cloudera.org:8080/#/c/7146/10/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java:

PS10, Line 36:  Kudu server-side components
I think it was clearer as 'masters and tablet servers'


http://gerrit.cloudera.org:8080/#/c/7146/10/java/kudu-client/src/main/java/org/apache/kudu/client/RpcHelper.java
File java/kudu-client/src/main/java/org/apache/kudu/client/RpcHelper.java:

Line 49:  * This is a stateless helper to send RPCs to a Kudu server.
It doesn't appear to be stateless, although it could easily be made so.  
Perhaps this class should just expose a single static method which takes the 
'client', 'connection' and 'rpc'?


Line 57: public class RpcHelper {
Is this the same as the Proxy on the c++ side?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [java-client] separating Connection

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new patch set (#10).

Change subject: [java-client] separating Connection
..

[java-client] separating Connection

This patch separates lower-level, connection-related functionality
from the TabletClient class into the new Connection class.
The TabletClient has been renamed into RpcHelper.  Also,
this patch contains other micro-updates on the related code.

In addition, this patch addresses KUDU-1878.

This work is done in the context of KUDU-2013.

Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
A java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.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/Negotiator.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/RpcHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
D java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITFaultTolerantScanner.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITNonFaultTolerantScanner.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.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/TestTimeouts.java
21 files changed, 1,398 insertions(+), 1,256 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/7146/10
-- 
To view, visit http://gerrit.cloudera.org:8080/7146
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [java-client] separating Connection

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [java-client] separating Connection
..


Patch Set 9: Verified+1

It seems TestKuduClient.testCloseShortlyAfterOpen was flaky before my changes, 
but I'll take a look at it anyway.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [java-client] separating Connection

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [java-client] separating Connection
..


Patch Set 8:

(3 comments)

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

PS8, Line 134: private
> nit: private comes first.
Done


Line 207:   @Nullable
> Seems weird to put these two methods before the ctor.
Done


PS8, Line 537: repeatably
> Typo?
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [java-client] separating Connection

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

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

to look at the new patch set (#9).

Change subject: [java-client] separating Connection
..

[java-client] separating Connection

This patch separates lower-level, connection-related functionality
from the TabletClient class into the new Connection class.
The TabletClient has been renamed into RpcHelper.  Also,
this patch contains other micro-updates on the related code.

In addition, this patch addresses KUDU-1878.

This work is done in the context of KUDU-2013.

Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
A java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.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/Negotiator.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/RpcHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
D java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITFaultTolerantScanner.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITNonFaultTolerantScanner.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.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/TestTimeouts.java
21 files changed, 1,399 insertions(+), 1,268 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/7146/9
-- 
To view, visit http://gerrit.cloudera.org:8080/7146
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [java-client] separating Connection

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: [java-client] separating Connection
..


Patch Set 8:

(3 comments)

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

PS8, Line 134: private
nit: private comes first.


Line 207:   @Nullable
Seems weird to put these two methods before the ctor.


PS8, Line 537: repeatably
Typo?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [java-client] separating Connection

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

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

to look at the new patch set (#8).

Change subject: [java-client] separating Connection
..

[java-client] separating Connection

This patch separates lower-level, connection-related functionality
from the TabletClient class into the new Connection class.
The TabletClient has been renamed into RpcHelper.  Also,
this patch contains other micro-updates on the related code.

In addition, this patch addresses KUDU-1878.

This work is done in the context of KUDU-2013.

Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
A java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.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/Negotiator.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/RpcHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
D java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITFaultTolerantScanner.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITNonFaultTolerantScanner.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.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/TestTimeouts.java
21 files changed, 1,384 insertions(+), 1,252 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/7146/8
-- 
To view, visit http://gerrit.cloudera.org:8080/7146
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [java-client] separating Connection

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

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

to look at the new patch set (#7).

Change subject: [java-client] separating Connection
..

[java-client] separating Connection

This patch separates connection-related stuff from the TabletClient
class.  The TabletClient has been renamed into RpcHelper.  Also,
this patch brings in other micro clean-ups in the related code.

In addition, this patch addresses KUDU-1878.

This work is done in the context of KUDU-2013.

Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
A java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.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/Negotiator.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/RpcHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
D java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITFaultTolerantScanner.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITNonFaultTolerantScanner.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.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/TestTimeouts.java
21 files changed, 1,407 insertions(+), 1,285 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/7146/7
-- 
To view, visit http://gerrit.cloudera.org:8080/7146
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id4ac81d9454631e7501c31576c24f85e968bb871
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon