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<Void>. 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<QueuedMessage> queued; > This can be a List<QueuedMessage> 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<QueuedMessage> queued; > This can be a List<QueuedMessage> Done Line 537: HashMap<Integer, Callback<Void, CallResponseInfo>> 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 <aser...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
