Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12715 )
Change subject: [java] Make the KuduScanner iterable ...................................................................... Patch Set 2: (6 comments) This is just a partial review, I'm interested in making sure the memory semantics are clean before moving onto the rest. http://gerrit.cloudera.org:8080/#/c/12715/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12715/2//COMMIT_MSG@18 PS2, Line 18: changed Nit: changes http://gerrit.cloudera.org:8080/#/c/12715/2//COMMIT_MSG@19 PS2, Line 19: craete create http://gerrit.cloudera.org:8080/#/c/12715/2//COMMIT_MSG@20 PS2, Line 20: while still sharing the underlying : data. IIUC, you get one RowResultIterator for every Scan RPC response, and one RowResult for every row in the RowResultIterator. So, isn't this still unsafe if you iterate across an entire scan and store every RowResult in a collection? Behind the scenes won't groups of RowResults become invalid the moment the "active" RowResultIterator is replaced by a new one, which lets the JVM GC the old RowResultIterator? I don't see any reference from a RowResult to a RowResultIterator, so what prevents the GC from cleaning up a RowResultIterator once it's no longer referenced from KuduScannerIterator.currentIterator? 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. ScanRequest is a nested class, so it has a reference to the AsyncKuduScanner itself. Seems like you could modify the scanner builder to configure reuseRowResult, store it in the AsyncKuduScanner at build() time, then pass that configured value into this factory method? 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@31 PS2, Line 31: public class KuduScanner implements Iterable<RowResult> { Should AsyncKuduScanner also be Iterable? 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@65 PS2, Line 65: reUseRowResult Nit: reuseRowResult would be a tad more clear. -- 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 04:20:05 +0000 Gerrit-HasComments: Yes
