Alexey Serbin has posted comments on this change. Change subject: [java] separating Connection ......................................................................
Patch Set 18: (18 comments) http://gerrit.cloudera.org:8080/#/c/7146/18/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: PS18, Line 228: helper > nit: here and below it says 'helper' instead of 'proxy' (leftover from an e Done PS18, Line 250: as > sentence ends abruptly here Done PS18, Line 730: Not connected to server, will retry later > is this error message still accurate? when would the above return null? it Good catch. RemoteTablet will return null if its tabletServers member is empty -- that can happen if RemoteTablet.removeTabletClient() has been called by AsyncKuduClient.invalidateTabletCache() method. http://gerrit.cloudera.org:8080/#/c/7146/18/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java File java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java: PS18, Line 84: masterHelper > rename to masterProxy Done http://gerrit.cloudera.org:8080/#/c/7146/18/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: PS18, Line 71: connection > a connection Done PS18, Line 73: RPCs in flights > RPCs in flight Done PS18, Line 86: public > odd to see this one public -- is it visible for testing? or should we have Yep, probably having a getter is better idea. Done. Line 110: /** Initial header sent by the client upon connection establishment. */ > nit: add newline before this Done PS18, Line 349: negotiationResult > this should be guarded by 'lock' right? oh, sure -- good catch! PS18, Line 578: @response > same Done PS18, Line 578: @exception > this is odd syntax for javadoc Done http://gerrit.cloudera.org:8080/#/c/7146/18/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: PS18, Line 52: the connection > new connections? Done PS18, Line 55: connection > connections? Done PS18, Line 110: messages > RPCs Done Line 132: List<Connection> getConnectionListCopy() { > maybe @VisibleForTesting? Well, it's also used by AsyncKuduClient to expose the connection set to tests (so, not only tests use it). But it will not hurt, sure. http://gerrit.cloudera.org:8080/#/c/7146/18/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: PS18, Line 134: byte > while we are at it, let's make this an int? Done http://gerrit.cloudera.org:8080/#/c/7146/18/java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java File java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java: PS18, Line 158: disconnect or shutdown client connection > update this? Done http://gerrit.cloudera.org:8080/#/c/7146/18/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java: PS18, Line 478: for (Iterator<Process> it = tserverProcesses.values().iterator(); it.hasNext(); ) { : final Process p = it.next(); : while (true) { : try { : destroyAndWaitForProcess(p); : break; : } catch (InterruptedException e) { : wasInterrupted = true; : // Not being polite here: ignore the request to interrupt and continue cleaning up. : LOG.info("ignoring request to interrupt; waiting for tablet server {} to exit", p); : } : } : } : tserverProcesses.clear(); > can we extract a function for this repeated block? 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: 18 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
