Todd Lipcon has posted comments on this change. Change subject: KUDU-2095 - Add `keepAlive` method to Java API ......................................................................
Patch Set 3: (14 comments) http://gerrit.cloudera.org:8080/#/c/7749/3//COMMIT_MSG Commit Message: PS3, Line 13: properties that keep track of the last response, however adding this to the Java : client looked invasive and I did not implement it. not sure if this is a lack of functionality or just due to different implementation style? PS3, Line 18: MiniClusters are running at the same time Check out how TestAuthnTokenReacquire sets flags, for example, to avoid this Line 23: than it is running here but was unable to find a flag to lower it other I think you're looking for 'scanner_gc_check_interval_us' http://gerrit.cloudera.org:8080/#/c/7749/3/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 843: final ServerInfo info = tablet.getReplicaSelectedServerInfo(keepAliveRequest.getReplicaSelection()); this seems relatively suspicious. I see you copied it from 'close' above, but are we guaranteed that this returns the same ServerInfo as the server which is processing our currently-open scanner? http://gerrit.cloudera.org:8080/#/c/7749/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java: Line 45: import com.sun.tools.doclets.formats.html.SourceToHTMLConverter; hrmm? this is causing a build failure Line 645: * Keep the current remote scanner alive. can we provide more docs on how this is meant to be used? Line 652: final Deferred<Status> d = nit: no wrap, can just combine with the 'return' below PS3, Line 672: getKeepAliveRequest nit: change to multiple lines. Add a javadoc. Line 761: final Tserver.ScannerKeepAliveRequestPB.Builder builder = Tserver.ScannerKeepAliveRequestPB.newBuilder(); wrap Line 781: String tsUUID) throws KuduException { nit: alignment Line 783: Tserver.ScannerKeepAliveResponsePB.Builder builder = Tserver.ScannerKeepAliveResponsePB.newBuilder(); nit: wrap Line 785: Tserver.ScannerKeepAliveResponsePB resp = builder.build(); nit: extra space Line 788: return new Pair<Status, Object>(Status.OK(), error); nit: this whole function is indented one too much http://gerrit.cloudera.org:8080/#/c/7749/3/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java: PS3, Line 62: * Keep the current scanner alive on the tablet server. : * @return Status of the RPC request : * @throws KuduException needs more clear docs -- To view, visit http://gerrit.cloudera.org:8080/7749 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: [email protected] Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
