Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12715 )
Change subject: [java] Make the KuduScanner iterable ...................................................................... Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/12715/2/java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java File java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java: http://gerrit.cloudera.org:8080/#/c/12715/2/java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java@365 PS2, Line 365: scanners do not time out > Nit: we can be more specific ("to ensure that this scanner will not time ou Done http://gerrit.cloudera.org:8080/#/c/12715/2/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/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@1030 PS2, Line 1030: // TODO: Find a clean way to plumb in reuseRowResult. > I recall having a similar discussion with Dan in the past. Like you, I made A separate configuration can be defined and passed around by the drivers. It's in those configurations where client/language specific configurations would make sense. My dilemma still exists. Putting reuseRowResult in the scanToken proto isn't useful for the c++ client. I think I will add a setter on the scanner and see how that looks. http://gerrit.cloudera.org:8080/#/c/12715/2/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/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java@169 PS2, Line 169: return new KuduScannerIterator(this, asyncScanner.getKeepAlivePeriodMs() ); > Nit: extra space towards the end. Done http://gerrit.cloudera.org:8080/#/c/12715/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScannerIterator.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduScannerIterator.java: http://gerrit.cloudera.org:8080/#/c/12715/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScannerIterator.java@27 PS2, Line 27: public class KuduScannerIterator implements Iterator<RowResult> { > Doc the class too? Done http://gerrit.cloudera.org:8080/#/c/12715/2/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java File java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java: http://gerrit.cloudera.org:8080/#/c/12715/2/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@49 PS2, Line 49: private Slice rowData; > I know my original confusion stemmed from lack of Java knowledge, but I thi Done http://gerrit.cloudera.org:8080/#/c/12715/2/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/2/java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java@72 PS2, Line 72: if (reUseRowResult && numRows != 0) { : this.sharedRowResult = new RowResult(this.schema, this.bs, this.indirectBs, -1); : } else { : this.sharedRowResult = null; : } > Nit: use a ternary? Done http://gerrit.cloudera.org:8080/#/c/12715/2/src/kudu/client/client.proto File src/kudu/client/client.proto: http://gerrit.cloudera.org:8080/#/c/12715/2/src/kudu/client/client.proto@111 PS2, Line 111: scanners do not time out. > Nit: "this scanner won't time out" 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: 2 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 19:49:13 +0000 Gerrit-HasComments: Yes
