Tony Foerster has posted comments on this change. ( http://gerrit.cloudera.org:8080/7749 )
Change subject: KUDU-2095 - Add scanner `keepAlive` RPC call to Java API ...................................................................... Patch Set 9: (5 comments) http://gerrit.cloudera.org:8080/#/c/7749/9/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/7749/9/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1006 PS9, Line 1006: > nit: double space Done http://gerrit.cloudera.org:8080/#/c/7749/9/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1007 PS9, Line 1007: defaultAdminOperationTimeoutMs > this seems like an odd choice of timeout. I think this defaults to be prett I'm not sure exactly what's reasonable here? I set it to a constant 10 seconds. I also considered setting it to some division of defaultAdminOperationTimeoutMs, that way it would be somewhat configurable (but also more complicated maybe than it needs to be). http://gerrit.cloudera.org:8080/#/c/7749/9/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1008 PS9, Line 1008: final ReplicaSelection replicaSelection = scanner.getReplicaSelection(); : final ServerInfo info = : tablet.getReplicaSelectedServerInfo(replicaSelection); > is this guaranteed to be the same replica that the scanner is already open I think the server is always the same but this behavior has been marked as a bug in the meantime: https://github.com/cloudera/kudu/blob/master/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java#L191 I opened a pull request for this here: https://gerrit.cloudera.org/#/c/11088/ I kept the replica returned non-random per RemoteTablet instance, this way I won't have to keep track of replicas within the scanner. I think this is ok because it's unlikely (let me know if this is a bad assumption) for any client to read from the same tablet concurrently. http://gerrit.cloudera.org:8080/#/c/7749/9/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: http://gerrit.cloudera.org:8080/#/c/7749/9/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java@75 PS9, Line 75: SetFaultTolerant > this is the C++ API name I shamelessly stole this from the C++ docs obviously. I need to double check that the behavior with isFaultTolerant is the same. http://gerrit.cloudera.org:8080/#/c/7749/9/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/7749/9/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@203 PS9, Line 203: 50% of the scanner ttl. > I dont see anywhere setting a short TTL on this test, am I missing somethin Not missing anything I removed mistakenly. I had been doing this for the entire test like https://gerrit.cloudera.org/#/c/7749/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java I also tried just spawning another minicluster https://gerrit.cloudera.org/#/c/7749/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java I am thinking I will move this to a new test class. -- To view, visit http://gerrit.cloudera.org:8080/7749 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713 Gerrit-Change-Number: 7749 Gerrit-PatchSet: 9 Gerrit-Owner: Tony Foerster <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Tony Foerster <[email protected]> Gerrit-Comment-Date: Tue, 31 Jul 2018 15:59:57 +0000 Gerrit-HasComments: Yes
