Grant Henke 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 2: (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 It really shouldn't be possible given a scan would need to have succeeded. And if a scan succeeded we already have looked up the ServerInfo for that remote tablet. I think we can handle this the same way closeScanner does and result in a no-op. 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: */ > Need a @return for the Deferred<Void>. Done http://gerrit.cloudera.org:8080/#/c/11436/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@824 PS1, Line 824: > Short Javadoc. Done 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 test I think as long as the result is a broken test we should be okay in the mean time. I added comments above each test method to ensure it is clear that the name is significant. As a follow up, it looks like there is a way I could write an annotation that adds flags to the mini-cluster on a per-test basis. I will look to do that as a follow up change. http://gerrit.cloudera.org:8080/#/c/11436/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@186 PS1, Line 186: */ > Is this long enough in TSAN environments? Could you loop your new tests in This was an accident from experimentation. I will set to match all the other tests. We cam look to use a global setting in a follow up patch. http://gerrit.cloudera.org:8080/#/c/11436/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@201 PS1, Line 201: } > Isn't the default flush mode AUTO_FLUSH_SYNC, thus obviating the need for t Done 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. E We do have AssertHelpers.assertEventuallyTrue. The problem is if we call scanner.nextRows() too many times we will run out of rows. I would also need to write a version thats "assertEventuallyException" I think. http://gerrit.cloudera.org:8080/#/c/11436/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@246 PS1, Line 246: KuduScanner scanner = new KuduScanner.KuduScannerBuilder(client, table) > Nit: move this to L255. Done http://gerrit.cloudera.org:8080/#/c/11436/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@265 PS1, Line 265: if (accum == numRows) { > tablets Done http://gerrit.cloudera.org:8080/#/c/11436/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@281 PS1, Line 281: scanner.keepAlive(); > It's possible for poor scheduling to lead to a full TTL of elapsed time bet Done -- 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: 2 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Comment-Date: Thu, 13 Sep 2018 19:18:59 +0000 Gerrit-HasComments: Yes
