Todd Lipcon 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 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 pretty long, maybe a short one is better? 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 on? it doesn't seem so, though I'm not super familiar with this code. Given that replica selection might be somehow randomized this might get fired to some different server? 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 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 something? -- 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 03:00:54 +0000 Gerrit-HasComments: Yes
