Todd Lipcon has posted comments on this change. Change subject: WIP [java-client] separating Connection ......................................................................
Patch Set 2: (13 comments) did a first pass. I think many of my comments may equally apply to the pre-refactored code, in which case feel free to just drop TODOs in place for any that you think are worth fixing later. http://gerrit.cloudera.org:8080/#/c/7146/2/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: PS2, Line 104: /** : * The class to represent RPC response received from the remote server. : * If the @exception is null, then it's a success case, otherwise it's an error. : * For the recoverable error case, the @exception is of RecoverableException type, : * otherwise it's of NonRecoverableException type. : */ : static final class CallResponseInfo { : public final CallResponse response; : public final KuduException exception; : : CallResponseInfo(CallResponse response, KuduException exception) { : this.response = response; : this.exception = exception; : } : } nit: it's a little weird to have this class definition (and the one below) interspersed between member variable definitions PS2, Line 124: queuedMessages some typo here PS2, Line 125: private final class QueuedMessage { should this be static? Line 136: private ArrayList<QueuedMessage> queuedMessages = Lists.newArrayList(); add javadoc PS2, Line 138: /** Read timeout for the connection (used by Netty's ReadTimeoutHandler) */ : private final long socketReadTimeoutMs; : /** Timer to monitor read timeouts for the connection (used by Netty's ReadTimeoutHandler) */ : private final HashedWheelTimer timer; : : /** Security context to use for connection negotiation. */ : private final SecurityContext securityContext; would be nice to reorder the final fields separately from those fields that are mutable and protected by the lock PS2, Line 188: lock.lock(); nit: this usually goes outside of the 'try' so that if for whatever reason this threw an exception and didn't acquire a lock, we wouldn't then try to unlock it PS2, Line 198: Nullable I wonder if we could actually make this illegal to call prior to a connection negotiation being successful, once this refactor is complete? just a thought, no action required PS2, Line 241: super.channelDisconnected(ctx, e); // Let the ReplayingDecoder cleanup. i think the comment here is stale, we don't use ReplayingDecoder anymore. Does SimpleChannelUpstreamHandler have any body for this function? PS2, Line 263: queuedMessages = Lists.newArrayList(); once we are in NEGOTIATED state, is queuedMessages ever used again? I wonder if we could null it out here? PS2, Line 270: sendCallToWire(qm.message, qm.cb); sendCallToWire says it's GuardedBy("lock") but here you've released it already PS2, Line 378: // Schedule (re)connecting to the server. : connect(); : } in your plans for a refactor are you going to attempt to simplify this lifecycle so that an existing connection doesn't reconnect? I think that would be a noble pursuit http://gerrit.cloudera.org:8080/#/c/7146/2/java/kudu-client/src/main/java/org/apache/kudu/client/RowErrorsAndOverflowStatus.java File java/kudu-client/src/main/java/org/apache/kudu/client/RowErrorsAndOverflowStatus.java: PS2, Line 24: queuedMessages errant find/replace? http://gerrit.cloudera.org:8080/#/c/7146/2/java/kudu-client/src/main/java/org/apache/kudu/client/SessionConfiguration.java File java/kudu-client/src/main/java/org/apache/kudu/client/SessionConfiguration.java: Line 134: * @return true if operations are queuedMessages, else false. other errant find/replaces in this file -- 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: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: 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
