Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11436 )
Change subject: KUDU-2095: [java] Add scanner keepAlive API to the Java client ...................................................................... Patch Set 1: (10 comments) http://gerrit.cloudera.org:8080/#/c/11436/1/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: http://gerrit.cloudera.org:8080/#/c/11436/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1046 PS1, Line 1046: final ServerInfo info = tablet.getReplicaSelectedServerInfo(keepAliveRequest.getReplicaSelection()); Is it possible for 'info' to be null? Seems like closeScanner handles that case. http://gerrit.cloudera.org:8080/#/c/11436/1/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: http://gerrit.cloudera.org:8080/#/c/11436/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@717 PS1, Line 717: public Deferred<Void> keepAlive() { Need a @return for the Deferred<Void>. http://gerrit.cloudera.org:8080/#/c/11436/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@824 PS1, Line 824: final class KeepAliveRequest extends KuduRpc<Void> { Short Javadoc. http://gerrit.cloudera.org:8080/#/c/11436/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java: http://gerrit.cloudera.org:8080/#/c/11436/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@85 PS1, Line 85: if ("testKeepAlive".equals(testName.getMethodName()) || : "testScannerExpiration".equals(testName.getMethodName()) : ) Is there a more robust way to do this, such that renaming one of these tests won't cause this code to be skipped? Perhaps we could annotate the two tests and look for the corresponding annotation here? http://gerrit.cloudera.org:8080/#/c/11436/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@186 PS1, Line 186: @Test(timeout = 10000) Is this long enough in TSAN environments? Could you loop your new tests in dist-test? http://gerrit.cloudera.org:8080/#/c/11436/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@201 PS1, Line 201: session.flush(); Isn't the default flush mode AUTO_FLUSH_SYNC, thus obviating the need for this? http://gerrit.cloudera.org:8080/#/c/11436/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@212 PS1, Line 212: // Wait for the scanner to time out. : Thread.sleep(SHORT_SCANNER_TTL_MS * 2); : : try { : scanner.nextRows(); : fail("Exception was not thrown when accessing an expired scanner"); : } catch (NonRecoverableException ex) { : assertThat(ex.getMessage(), containsString("Scanner not found")); : } Would be nice to wrap this up in an equivalent to C++'s AssertEventually. Even though the test waits twice as long as the TTL, it's possible (especially in TSAN environments) for the scanner GC thread to not yet be scheduled, and for the test to fail. Anyway, if you can jury-rig an AssertEventually, you could omit the Thread.sleep() altogether. http://gerrit.cloudera.org:8080/#/c/11436/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@246 PS1, Line 246: int accum = 0; Nit: move this to L255. http://gerrit.cloudera.org:8080/#/c/11436/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@265 PS1, Line 265: // Ensure we actually end up between tablet. tablets http://gerrit.cloudera.org:8080/#/c/11436/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@281 PS1, Line 281: Thread.sleep(SHORT_SCANNER_TTL_MS / 2); It's possible for poor scheduling to lead to a full TTL of elapsed time between keep alive requests. Consider raising the TTL to more than 1s for this test, sleeping for less time, and looping for more iterations. Loop in dist-test with TSAN binaries to see how robust it is. -- To view, visit http://gerrit.cloudera.org:8080/11436 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic802f556c8860cdd43ef5f794c8f3658259bd0be Gerrit-Change-Number: 11436 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Comment-Date: Thu, 13 Sep 2018 18:29:11 +0000 Gerrit-HasComments: Yes
