You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Grant Henke (Code Review)" <ge...@cloudera.org> on 2019/03/11 16:34:22 UTC
[kudu-CR] [java] Make the KuduScanner iterable
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12715
Change subject: [java] Make the KuduScanner iterable
......................................................................
[java] Make the KuduScanner iterable
This patch makes the KuduScanner iterable to simplify
the API for processing the RowResults. It also pushes
the keep alive calls down into the KuduScannerIterator
so that any user of the KuduClient can automatically
leverage the API the same way the Spark integration does.
I have replaced as many places where the scanner
is iterated as I can find with this new implementation.
The Spark implementations were replaced as well, but
a special hasNext method with a callback was required.
Change-Id: I3e4ac59e30d0562c0a381d5e304af1dcfdcf5a1a
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
M java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java
A java/kudu-client/src/main/java/org/apache/kudu/client/KuduScannerIterator.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestFlexiblePartitioning.java
M java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/ClientTestUtil.java
M src/kudu/client/client.proto
15 files changed, 176 insertions(+), 127 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/12715/1
--
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: newchange
Gerrit-Change-Id: I3e4ac59e30d0562c0a381d5e304af1dcfdcf5a1a
Gerrit-Change-Number: 12715
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
[kudu-CR] [java] Make the KuduScanner iterable
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12715 )
Change subject: [java] Make the KuduScanner iterable
......................................................................
Patch Set 2:
I posted an update to get some feedback on the overall approach. I still need to figure out how to plumb reuseRowResult all the way through and write some good tests.
If you have a suggestion on plumbing reuseRowResult let me know.
--
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 <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 12 Mar 2019 02:57:59 +0000
Gerrit-HasComments: No
[kudu-CR] [java] Make the KuduScanner iterable
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12715 )
Change subject: [java] Make the KuduScanner iterable
......................................................................
Patch Set 6: Code-Review+2
--
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: 6
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 13 Mar 2019 03:34:05 +0000
Gerrit-HasComments: No
[kudu-CR] [java] Make the KuduScanner iterable
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Mike Percy, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12715
to look at the new patch set (#6).
Change subject: [java] Make the KuduScanner iterable
......................................................................
[java] Make the KuduScanner iterable
This patch makes the KuduScanner iterable to simplify
the API for processing the RowResults. It also pushes
the keep alive calls down into the KuduScannerIterator
so that any user of the KuduClient can automatically
leverage the API the same way the Spark integration does.
The existing RowResultIterator implementation reuses
the same RowResult object for all rows in the batch.
This can cause unexpected behavior when storing
RowResults from the scanner. This patch changes
the RowResultIterator to instead create a RowResult
object for every row, while still sharing the underlying
data.
I have replaced as many places where the scanner
is iterated as I can find with this new implementation.
The Spark implementations were replaced as well, but
a special hasNext method with a callback was required.
Change-Id: I3e4ac59e30d0562c0a381d5e304af1dcfdcf5a1a
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
M java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java
A java/kudu-client/src/main/java/org/apache/kudu/client/KuduScannerIterator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java
M java/kudu-client/src/main/java/org/apache/kudu/util/Slice.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestFlexiblePartitioning.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java
M java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/ClientTestUtil.java
M src/kudu/client/client.proto
19 files changed, 413 insertions(+), 157 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/12715/6
--
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: newpatchset
Gerrit-Change-Id: I3e4ac59e30d0562c0a381d5e304af1dcfdcf5a1a
Gerrit-Change-Number: 12715
Gerrit-PatchSet: 6
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [java] Make the KuduScanner iterable
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12715 )
Change subject: [java] Make the KuduScanner iterable
......................................................................
[java] Make the KuduScanner iterable
This patch makes the KuduScanner iterable to simplify
the API for processing the RowResults. It also pushes
the keep alive calls down into the KuduScannerIterator
so that any user of the KuduClient can automatically
leverage the API the same way the Spark integration does.
The existing RowResultIterator implementation reuses
the same RowResult object for all rows in the batch.
This can cause unexpected behavior when storing
RowResults from the scanner. This patch changes
the RowResultIterator to instead create a RowResult
object for every row, while still sharing the underlying
data.
I have replaced as many places where the scanner
is iterated as I can find with this new implementation.
The Spark implementations were replaced as well, but
a special hasNext method with a callback was required.
Change-Id: I3e4ac59e30d0562c0a381d5e304af1dcfdcf5a1a
Reviewed-on: http://gerrit.cloudera.org:8080/12715
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
M java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java
A java/kudu-client/src/main/java/org/apache/kudu/client/KuduScannerIterator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java
M java/kudu-client/src/main/java/org/apache/kudu/util/Slice.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestFlexiblePartitioning.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java
M java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/ClientTestUtil.java
M src/kudu/client/client.proto
19 files changed, 413 insertions(+), 157 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
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: merged
Gerrit-Change-Id: I3e4ac59e30d0562c0a381d5e304af1dcfdcf5a1a
Gerrit-Change-Number: 12715
Gerrit-PatchSet: 7
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [java] Make the KuduScanner iterable
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 12 Mar 2019 19:49:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Make the KuduScanner iterable
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 12 Mar 2019 22:09:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Make the KuduScanner iterable
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12715 )
Change subject: [java] Make the KuduScanner iterable
......................................................................
Patch Set 4:
(9 comments)
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: throw new NonRecoverableException(statusIncomplete);
> A separate configuration can be defined and passed around by the drivers. I
Right, in this case it sounds like something outside the scan token makes more sense, because:
1) It's only an issue for the Java client, whose different memory semantics enable this user-configurable tradeoff.
2) It _is_ something that executors would care about (not drivers), because it affects how the scanner-consuming code should be written.
I think a scanner setter is fine, though I might reduce the visibility/audience a bit (i.e. not fully "stable" or whatever) since it's a bit esoteric.
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: if the RowResults
: * will not be stored between calls to {@link RowResultIterator#next()).
I think this last part needs to state the limitations more clearly and more loudly. How about something like:
This can be a useful optimization to reduce the number of objects created.
Note: DO NOT use this if the RowResult is stored between calls to next(). Enabling this optimization
means that a call to next() invalidates the previously returned RowResult; accessing it after next() (by e.g.
storing all RowResults in a collection and accessing them later) will lead to <whatever bad stuff happens>
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 if the RowResults
: * will not be stored between calls to {@link RowResultIterator#next()).
See what I wrote in AsyncKuduScanner.
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: this.reuseRowResult = reuseRowResult;
You don't actually need this.reuseRowResult; seems like you could get by with checking whether this.sharedRowResult is null or not.
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 don't need the explicit flush on L71.
http://gerrit.cloudera.org:8080/#/c/12715/4/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@73
PS4, Line 73: // Ensure a java foreach works on the iterable scanner.
Maybe it'd be clearer as "Ensure that when an enhanced for-loop is used, there's no sharing of RowResult objects."
http://gerrit.cloudera.org:8080/#/c/12715/4/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@82
PS4, Line 82: // Create a scanner with the reuseRowResult optimization.
Then you can juxtapose this comment with the one above (that when reuseRowResult=true, RowResult objects are shared).
http://gerrit.cloudera.org:8080/#/c/12715/4/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@102
PS4, Line 102: String tableName = "testKeepAlive";
Any reason you can't reuse the class member tableName instead?
http://gerrit.cloudera.org:8080/#/c/12715/4/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@105
PS4, Line 105: new ColumnSchema.ColumnSchemaBuilder("val", Type.INT32).build());
Could remove this column; doesn't seem like it's relevant for the test.
--
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: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 12 Mar 2019 21:07:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Make the KuduScanner iterable
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Mike Percy, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12715
to look at the new patch set (#5).
Change subject: [java] Make the KuduScanner iterable
......................................................................
[java] Make the KuduScanner iterable
This patch makes the KuduScanner iterable to simplify
the API for processing the RowResults. It also pushes
the keep alive calls down into the KuduScannerIterator
so that any user of the KuduClient can automatically
leverage the API the same way the Spark integration does.
The existing RowResultIterator implementation reuses
the same RowResult object for all rows in the batch.
This can cause unexpected behavior when storing
RowResults from the scanner. This patch changes
the RowResultIterator to instead create a RowResult
object for every row, while still sharing the underlying
data.
I have replaced as many places where the scanner
is iterated as I can find with this new implementation.
The Spark implementations were replaced as well, but
a special hasNext method with a callback was required.
Change-Id: I3e4ac59e30d0562c0a381d5e304af1dcfdcf5a1a
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
M java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java
A java/kudu-client/src/main/java/org/apache/kudu/client/KuduScannerIterator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java
M java/kudu-client/src/main/java/org/apache/kudu/util/Slice.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestFlexiblePartitioning.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java
M java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/ClientTestUtil.java
M src/kudu/client/client.proto
19 files changed, 414 insertions(+), 157 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/12715/5
--
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: newpatchset
Gerrit-Change-Id: I3e4ac59e30d0562c0a381d5e304af1dcfdcf5a1a
Gerrit-Change-Number: 12715
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [java] Make the KuduScanner iterable
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Mike Percy, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12715
to look at the new patch set (#2).
Change subject: [java] Make the KuduScanner iterable
......................................................................
[java] Make the KuduScanner iterable
This patch makes the KuduScanner iterable to simplify
the API for processing the RowResults. It also pushes
the keep alive calls down into the KuduScannerIterator
so that any user of the KuduClient can automatically
leverage the API the same way the Spark integration does.
The existing RowResultIterator implementation reuses
the same RowResult object for all rows in the batch.
This can cause unexpected behavior when storing
RowResults from the scanner. This patch changed
the RowResultIterator to instead craete a RowResult
object for every row, while still sharing the underlying
data.
I have replaced as many places where the scanner
is iterated as I can find with this new implementation.
The Spark implementations were replaced as well, but
a special hasNext method with a callback was required.
Change-Id: I3e4ac59e30d0562c0a381d5e304af1dcfdcf5a1a
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
M java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java
A java/kudu-client/src/main/java/org/apache/kudu/client/KuduScannerIterator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestFlexiblePartitioning.java
M java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/ClientTestUtil.java
M src/kudu/client/client.proto
17 files changed, 214 insertions(+), 156 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/12715/2
--
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: newpatchset
Gerrit-Change-Id: I3e4ac59e30d0562c0a381d5e304af1dcfdcf5a1a
Gerrit-Change-Number: 12715
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [java] Make the KuduScanner iterable
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12715 )
Change subject: [java] Make the KuduScanner iterable
......................................................................
Patch Set 1:
There are a ton of existing write ups on Java GC and long-lived vs short lived objects. Here is one of the top results from a quick google: https://www.bettercodebytes.com/the-cost-of-object-creation-in-java-including-garbage-collection/
I don't think this is something we need to do extensive testing on ourselves, unless it's for personal interest.
--
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: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 11 Mar 2019 19:21:11 +0000
Gerrit-HasComments: No
[kudu-CR] [java] Make the KuduScanner iterable
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12715 )
Change subject: [java] Make the KuduScanner iterable
......................................................................
Patch Set 1:
> Patch Set 1:
>
> The more I think about this, the more I still think it is useful.
>
> I assume the optimization is to prevent having 1 object per row result, so sharing the actual data isn't enough. If 1 object pre row isn't a problem this could be fixed.
Yea, that's my suggestion above. Create one object per row, and make sure that "next batch" also gets a new buffer (I think it already does?). Let's make the default behavior "safe".
There's still a potential foot-gun in terms of memory blowup that each RowResult holds a reference to the whole batch of results, but I think that one's a little less likely to cause problems, and we could offer a 'RowResult.detachMemoryFromBatch()' or something as a workaround.
--
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: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 11 Mar 2019 17:25:08 +0000
Gerrit-HasComments: No
[kudu-CR] [java] Make the KuduScanner iterable
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12715 )
Change subject: [java] Make the KuduScanner iterable
......................................................................
Patch Set 1:
Interesting, I had testing iterating the results, but I did not test storing the results. Storing the results is a real issue.
It's a bummer that everyone who uses the KuduScanner ends up implementing some form of an iterator. With KeepAlive support that copy/pasted iterator pattern gets even worse.
--
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: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 11 Mar 2019 16:46:21 +0000
Gerrit-HasComments: No
[kudu-CR] [java] Make the KuduScanner iterable
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12715 )
Change subject: [java] Make the KuduScanner iterable
......................................................................
Patch Set 1:
Perhaps we could change the default behavior to actually copy RowResults rather than reusing them. I'm not sure the reuse optimization is really such a huge win in modern JVMs which can probably escape-analysis optimize it away... we can add a flag to reuse for advanced users, since I think even without an iterator API people probably make the mistake of shoving RowResults in a list. Probably better to be foot-gun-proof than avoiding a optimization.
--
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: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 11 Mar 2019 16:49:24 +0000
Gerrit-HasComments: No
[kudu-CR] [java] Make the KuduScanner iterable
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 12 Mar 2019 04:20:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Make the KuduScanner iterable
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12715 )
Change subject: [java] Make the KuduScanner iterable
......................................................................
Patch Set 3:
(8 comments)
Would be good to make sure that we still have test coverage over the "old way" of iterating too.
http://gerrit.cloudera.org:8080/#/c/12715/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12715/2//COMMIT_MSG@20
PS2, Line 20: while still sharing the underlying
: data.
> The row data returned from a Scan RPC is passed by reference to the RowResu
Grant and I discussed this offline.
I had assumed that the semantics for the Java Slice class mirrored that of the C++ Slice class, where it's the caller's responsibility to ensure that data pointed to by a Slice remains live for the lifetime of the Slice. But in Java, arrays are objects, not primitives, and so when a Java Slice points to an array of data, that array is considered live (from the GC's perspective).
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 out").
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.
> The challenge here is that I would need to add this property to the scanTok
I recall having a similar discussion with Dan in the past. Like you, I made the case that scan tokens should contain just enough information to describe "what" is being scanned, and the rest should be set by whomever is hydrating the token. Dan's counter-argument is that even if that separation makes sense from an abstraction standpoint, it's not particularly practical because often times the "how" to scan is only known by the driver; if we force the "how" to be set by executors, the driver now needs to send that information via side channel to the executors.
Practicality carried the day: scan tokens are certainly richer than they need to be, but easier to use.
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.
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?
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 think it'd be useful to doc the reference semantics that tripped me here, to explain how RowResult effectively has a reference to the data. Maybe that's better served by documenting the surprising (to a C++ developer) semantics in Slice itself.
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?
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"
--
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: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 12 Mar 2019 18:30:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Make the KuduScanner iterable
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12715 )
Change subject: [java] Make the KuduScanner iterable
......................................................................
Patch Set 1: Code-Review-1
I seem to remember we considered this several years ago and rejected it due to some object reuse optimizations -- for example RowResultIterator actually yields the same RowResult object repeatedly, and just advances it. This makes it unsafe to use as a general iterator -- eg new ArrayList<>(myScanner) would end up with an array cotaining a bunch of copies of the same row, rather than the expected results. If you look at the RowResult class it actually says it's not safe to store.
Did I miss somewhere where this patch handles this?
--
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: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 11 Mar 2019 16:40:48 +0000
Gerrit-HasComments: No
[kudu-CR] [java] Make the KuduScanner iterable
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12715 )
Change subject: [java] Make the KuduScanner iterable
......................................................................
Patch Set 1:
The more I think about this, the more I still think it is useful.
I assume the optimization is to prevent having 1 object per row result, so sharing the actual data isn't enough. If 1 object pre row isn't a problem this could be fixed.
Even if it can't be fixed, we can still make KuduScanner iterable, and it would be just as surprising as RowResult being a shared/unstorable object. Given most uses will implement some form of iteration, I suspect this will prevent more bugs than it will introduce. And of course clear documentation about storage can be added.
--
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: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 11 Mar 2019 16:55:00 +0000
Gerrit-HasComments: No
[kudu-CR] [java] Make the KuduScanner iterable
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12715 )
Change subject: [java] Make the KuduScanner iterable
......................................................................
Patch Set 2:
(6 comments)
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
Done
http://gerrit.cloudera.org:8080/#/c/12715/2//COMMIT_MSG@19
PS2, Line 19: craete
> create
Done
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 Ro
The row data returned from a Scan RPC is passed by reference to the RowResult and then the RowResult has an index to its row in that data. Every-time a new Scan RPC Response, and therefore RowResultIterator, is created a new Slice/array is used for the new underlying data.
Java won't GC any of the batch data until no references are still being used. Once the last RowResult for a batch is GCd, so will the underlying data.
I will have tests that prove this.
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 AsyncKuduScanne
The challenge here is that I would need to add this property to the scanToken to make it work with the scanToken API. Since this is a Java only thing that didn't really make sense to me.
I think we have bloated the scanToken proto already. In a perfect world I think a scanToken would define *what* to scan, and some other configs could define *how*.
I could provide a setter method on the Scanner object after it's created, it's just not my favorite style.
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?
I could, but a use wasn't really clear.
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.
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 <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 12 Mar 2019 14:11:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Make the KuduScanner iterable
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12715 )
Change subject: [java] Make the KuduScanner iterable
......................................................................
Patch Set 5: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12715/5/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/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@420
PS5, Line 420: RowResult after a call next()
Nit: previously returned RowResult after a call to next()
(previously returned disambiguates; don't want people to think that they can't access the RowResult _returned_ by next()!)
http://gerrit.cloudera.org:8080/#/c/12715/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@422
PS5, Line 422: to represent
Nit: as per
--
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 <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 12 Mar 2019 22:14:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Make the KuduScanner iterable
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Mike Percy, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12715
to look at the new patch set (#3).
Change subject: [java] Make the KuduScanner iterable
......................................................................
[java] Make the KuduScanner iterable
This patch makes the KuduScanner iterable to simplify
the API for processing the RowResults. It also pushes
the keep alive calls down into the KuduScannerIterator
so that any user of the KuduClient can automatically
leverage the API the same way the Spark integration does.
The existing RowResultIterator implementation reuses
the same RowResult object for all rows in the batch.
This can cause unexpected behavior when storing
RowResults from the scanner. This patch changes
the RowResultIterator to instead create a RowResult
object for every row, while still sharing the underlying
data.
I have replaced as many places where the scanner
is iterated as I can find with this new implementation.
The Spark implementations were replaced as well, but
a special hasNext method with a callback was required.
Change-Id: I3e4ac59e30d0562c0a381d5e304af1dcfdcf5a1a
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
M java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java
A java/kudu-client/src/main/java/org/apache/kudu/client/KuduScannerIterator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestFlexiblePartitioning.java
M java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/ClientTestUtil.java
M src/kudu/client/client.proto
17 files changed, 214 insertions(+), 156 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/12715/3
--
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: newpatchset
Gerrit-Change-Id: I3e4ac59e30d0562c0a381d5e304af1dcfdcf5a1a
Gerrit-Change-Number: 12715
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [java] Make the KuduScanner iterable
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Mike Percy, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12715
to look at the new patch set (#4).
Change subject: [java] Make the KuduScanner iterable
......................................................................
[java] Make the KuduScanner iterable
This patch makes the KuduScanner iterable to simplify
the API for processing the RowResults. It also pushes
the keep alive calls down into the KuduScannerIterator
so that any user of the KuduClient can automatically
leverage the API the same way the Spark integration does.
The existing RowResultIterator implementation reuses
the same RowResult object for all rows in the batch.
This can cause unexpected behavior when storing
RowResults from the scanner. This patch changes
the RowResultIterator to instead create a RowResult
object for every row, while still sharing the underlying
data.
I have replaced as many places where the scanner
is iterated as I can find with this new implementation.
The Spark implementations were replaced as well, but
a special hasNext method with a callback was required.
Change-Id: I3e4ac59e30d0562c0a381d5e304af1dcfdcf5a1a
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
M java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java
A java/kudu-client/src/main/java/org/apache/kudu/client/KuduScannerIterator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java
M java/kudu-client/src/main/java/org/apache/kudu/util/Slice.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestFlexiblePartitioning.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java
M java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/ClientTestUtil.java
M src/kudu/client/client.proto
19 files changed, 405 insertions(+), 157 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/12715/4
--
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: newpatchset
Gerrit-Change-Id: I3e4ac59e30d0562c0a381d5e304af1dcfdcf5a1a
Gerrit-Change-Number: 12715
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [java] Make the KuduScanner iterable
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12715 )
Change subject: [java] Make the KuduScanner iterable
......................................................................
Patch Set 1:
> > I assume the optimization is to prevent having 1 object per row result, so sharing the actual data isn't enough. If 1 object pre row isn't a problem this could be fixed.
>
> Yea, that's my suggestion above. Create one object per row, and make sure that "next batch" also gets a new buffer (I think it already does?). Let's make the default behavior "safe".
Could we benchmark this, to test our assumption about modern JVMs handling this more efficiently?
--
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: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 11 Mar 2019 18:07:06 +0000
Gerrit-HasComments: No