Todd Lipcon 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 earlier revision) PS18, Line 250: as sentence ends abruptly here PS18, Line 730: Not connected to server, will retry later is this error message still accurate? when would the above return null? it no longer seems to be related to a connection-cache miss 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 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 PS18, Line 73: RPCs in flights RPCs in flight PS18, Line 86: public odd to see this one public -- is it visible for testing? or should we have a getter? Line 110: /** Initial header sent by the client upon connection establishment. */ nit: add newline before this PS18, Line 349: negotiationResult this should be guarded by 'lock' right? PS18, Line 578: @response same PS18, Line 578: @exception this is odd syntax for javadoc 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? PS18, Line 55: connection connections? PS18, Line 110: messages RPCs Line 132: List<Connection> getConnectionListCopy() { maybe @VisibleForTesting? 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? 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? 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? -- 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
