Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12715 )
Change subject: [java] Make the KuduScanner iterable ...................................................................... Patch Set 5: (8 comments) http://gerrit.cloudera.org:8080/#/c/12715/4/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/12715/4/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@416 PS4, Line 416: created. : * > I think this last part needs to state the limitations more clearly and more Done http://gerrit.cloudera.org:8080/#/c/12715/4/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/12715/4/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java@49 PS4, Line 49: : * This can be a useful optimization to reduce the number of objects created. : * > See what I wrote in AsyncKuduScanner. Done http://gerrit.cloudera.org:8080/#/c/12715/4/java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java File java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java: http://gerrit.cloudera.org:8080/#/c/12715/4/java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java@71 PS4, Line 71: new RowResult(this.schema, this.bs, this.indirectBs, -1) : null; > You don't actually need this.reuseRowResult; seems like you could get by wi Done http://gerrit.cloudera.org:8080/#/c/12715/4/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java: http://gerrit.cloudera.org:8080/#/c/12715/4/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@63 PS4, Line 63: KuduSession session = client.newSession(); > Isn't the default mode for a new session AUTO_FLUSH_SYNC? In which case you Done http://gerrit.cloudera.org:8080/#/c/12715/4/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@73 PS4, Line 73: > Maybe it'd be clearer as "Ensure that when an enhanced for-loop is used, th Done http://gerrit.cloudera.org:8080/#/c/12715/4/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@82 PS4, Line 82: assertEquals(numRows, results.size()); > Then you can juxtapose this comment with the one above (that when reuseRowR Done http://gerrit.cloudera.org:8080/#/c/12715/4/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@102 PS4, Line 102: int shortScannerTtlMs = 5000; > Any reason you can't reuse the class member tableName instead? Done http://gerrit.cloudera.org:8080/#/c/12715/4/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@105 PS4, Line 105: Schema tableSchema = new Schema(Collections.singletonList( > Could remove this column; doesn't seem like it's relevant for the test. Done -- To view, visit http://gerrit.cloudera.org:8080/12715 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3e4ac59e30d0562c0a381d5e304af1dcfdcf5a1a Gerrit-Change-Number: 12715 Gerrit-PatchSet: 5 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Tue, 12 Mar 2019 22:09:36 +0000 Gerrit-HasComments: Yes
