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/07 15:51:57 UTC
[kudu-CR] POC: [java] Add diff scan support
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12689
Change subject: POC: [java] Add diff scan support
......................................................................
POC: [java] Add diff scan support
Plumbs the diff scan support into the java client. This is
a quick poc patch that adds minimal support and a
rought test. The test is currently failing because
the is_deleted column is being set to false.
Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
---
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
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/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScannerMultiTablet.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala
M src/kudu/client/client.proto
12 files changed, 297 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/12689/1
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
[kudu-CR] [java] Add private diff scan support
Posted by "Greg Solovyev (Code Review)" <ge...@cloudera.org>.
Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/12689 )
Change subject: [java] Add private diff scan support
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12689/5/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/12689/5/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@54
PS5, Line 54: private static final Logger LOG = LoggerFactory.getLogger(TestScannerMultiTablet.class);
> This is used quite a bit in testDiffScan below.
my bad. Browser search was not working well in the old UI
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
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: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 14 Mar 2019 21:10:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Add private diff scan support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12689 )
Change subject: [java] Add private diff scan support
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12689/12/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/12689/12/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@193
PS12, Line 193: physical time
> Incrementing the physical time doesn't seem right; doesn't it allow for 'mu
Ah, that makes sense. Incrementing the encoded time is effectively incrementing the logical time.
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
Gerrit-PatchSet: 12
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Tue, 19 Mar 2019 04:07:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Add private diff scan support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo, Greg Solovyev,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12689
to look at the new patch set (#8).
Change subject: [java] Add private diff scan support
......................................................................
[java] Add private diff scan support
Adds a private diff scan API to the java client. This is
achieved primarily by adding a
`diffScan(long startTimestamp, long endTimestamp)`
method to the AbstractKuduScannerBuilder builder,
and then configuring the scan RPC to include the set fields.
The column projection of the scan will include an extra
“is_deleted” column when a diff scan is performed.
This column can be used like a standard boolean
column to detect if the row was deleted.
Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
---
M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.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
M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
M java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java
M java/kudu-client/src/main/java/org/apache/kudu/util/TimestampUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java
M src/kudu/client/client.proto
13 files changed, 424 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/12689/8
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
Gerrit-PatchSet: 8
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [java] Add private diff scan support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo, Greg Solovyev,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12689
to look at the new patch set (#12).
Change subject: [java] Add private diff scan support
......................................................................
[java] Add private diff scan support
Adds a private diff scan API to the java client. This is
achieved primarily by adding a
`diffScan(long startTimestamp, long endTimestamp)`
method to the AbstractKuduScannerBuilder builder,
and then configuring the scan RPC to include the set fields.
The column projection of the scan will include an extra
“is_deleted” column when a diff scan is performed.
This column can be used like a standard boolean
column to detect if the row was deleted.
Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
---
M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.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
M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
M java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java
M java/kudu-client/src/main/java/org/apache/kudu/util/TimestampUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java
M src/kudu/client/client.proto
13 files changed, 433 insertions(+), 53 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/12689/12
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
Gerrit-PatchSet: 12
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [java] Add private diff scan support
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12689 )
Change subject: [java] Add private diff scan support
......................................................................
Patch Set 7:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
File java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java:
PS6:
> I don't think physical type makes sense. That is primarily used server side
Yeah I guess virtualType works.
http://gerrit.cloudera.org:8080/#/c/12689/6/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/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@323
PS6, Line 323: if (startTimestamp != AsyncKuduClient.NO_TIMESTAMP) {
> Not sure honestly...this was the existing behavior. I am not totally sure w
Yeah I think that's worth investigating:
1. What does stripping achieve?
2. Why can we get away with not stripping these?
Git history may help.
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@355
PS6, Line 355: .nullable(columnToClone.isNullable())
> Do you know why we need to strip the column schema?
No clue; presumably if we pass along too much information back to the server, it complains?
I took a look and the server will complain if the schema has column IDs in it, but the ColumnSchema Java class doesn't even store those.
http://gerrit.cloudera.org:8080/#/c/12689/6/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/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@244
PS6, Line 244: resultNumDeletes++;
> It means a key was in the results of the scanner, but not in the original m
I'd rephrase the comment to describe what this condition means w.r.t. the test's expectations. Something like "The key was not found in the mutations map. This means that we somehow managed to scan a row that was never mutated. It's an error and will trigger an assert below."
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@264
PS6, Line 264: private Map<Integer, ChangeType> generateMutations(
> I can, this just seemed like a convenient place to contain a "session". Is
Just less overhead, but it's not a huge deal if it'll contort the test.
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@325
PS6, Line 325:
> INSERT will still be a valid endType, some mutations will occur on the orig
I see where the disconnect is: I assumed that endType==INSERT means the row should not have been mutated. But in this context it actually means REINSERT; that is, this is a row whose final state is an "INSERT" because there was a DELETE right before it, and some number of UPDATE/DELETE/REINSERT before that.
Maybe you can clarify that somewhere here with a comment?
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@337
PS6, Line 337: }
> I still like to close sessions when I am done with them. I think it's a goo
When you said closable you set off my spidey sense: how do you feel about opening the session via try-with-resources so that we'll still close it if something here throws?
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@347
PS6, Line 347: }
: assertFalse(resp.hasRowError());
: return row.getInt(0);
: }
> I broke it out so that the error would be printed when the test fails, othe
But don't assertion failures result in a displayed stack trace beginning with an AssertionError? And if you use the assertFalse(String, condition) variant as I suggested, won't that stack trace's AssertionError include the String?
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
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: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Sat, 16 Mar 2019 04:40:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Add private diff scan support
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12689 )
Change subject: [java] Add private diff scan support
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12689/12/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/12689/12/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@193
PS12, Line 193: physical time
Incrementing the physical time doesn't seem right; doesn't it allow for 'mutations' to be assigned a timestamp _below_ startHT (i.e. same physical time but higher logical time)? Why not increment the logical time? Or better yet, how about incrementing the _encoded_ timestamp by 1 without decoding it at all? There's a snippet in client.h (it's been removed by Hao on master) that suggests this is OK:
/// How to get Read-Your-Writes consistency:
/// the code snippet below uses KuduClient::GetLatestObservedTimestamp() along
/// with KuduScanner::SetSnapshotRaw() to perform READ_AT_SNAPSHOT scan
/// containing the data which has just been written. Notice extra 1
/// added to the timestamp passed to KuduScanner::SetSnapshotRaw():
/// @code
/// shared_ptr<KuduClient> client;
/// ... // open/initialize the client
/// shared_ptr<KuduSession> session(client->NewSession());
/// ... // set Kudu session properties
/// shared_ptr<KuduTable> table;
/// ... // open the table
/// unique_ptr<KuduInsert> insert_op(table->NewInsert());
/// ... // populate new insert operation with data
/// RETURN_NOT_OK(session->Apply(insert_op.release()));
/// RETURN_NOT_OK(session->Flush());
/// uint64_t snapshot_timestamp = client->GetLatestObservedTimestamp() + 1;
/// KuduScanner scanner(table.get());
/// RETURN_NOT_OK(scanner.SetSnapshotRaw(snapshot_timestamp));
/// RETURN_NOT_OK(scanner.SetSelection(KuduClient::LEADER_ONLY));
/// RETURN_NOT_OK(scanner.SetReadMode(KuduScanner::READ_AT_SNAPSHOT));
/// RETURN_NOT_OK(scanner.Open());
/// ... // retrieve scanned rows
/// @endcode
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
Gerrit-PatchSet: 12
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Tue, 19 Mar 2019 04:03:06 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Add private diff scan support
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12689 )
Change subject: [java] Add private diff scan support
......................................................................
Patch Set 14:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/12689/14/java/kudu-client/src/main/java/org/apache/kudu/Schema.java
File java/kudu-client/src/main/java/org/apache/kudu/Schema.java:
http://gerrit.cloudera.org:8080/#/c/12689/14/java/kudu-client/src/main/java/org/apache/kudu/Schema.java@70
PS14, Line 70: private final boolean hasIsDeleted;
: private final int isDeletedIndex;
If an index value of -1 means "no index", you shouldn't need both of these.
http://gerrit.cloudera.org:8080/#/c/12689/14/java/kudu-client/src/main/java/org/apache/kudu/Schema.java@137
PS14, Line 137: if(column.getWireType() == Common.DataType.IS_DELETED) {
Nit: if (
http://gerrit.cloudera.org:8080/#/c/12689/14/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/12689/14/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@156
PS14, Line 156: static final String DEFAULT_IS_DELETED_COL_NAME = "_kudu_is_deleted";
Could we make the default "is_deleted"? Even that seems pretty collision-proof, and I think it'd be more readable if someone snoops the transfer.
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
Gerrit-PatchSet: 14
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 21 Mar 2019 21:58:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Add private diff scan support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo, Greg Solovyev,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12689
to look at the new patch set (#7).
Change subject: [java] Add private diff scan support
......................................................................
[java] Add private diff scan support
Adds a private diff scan API to the java client. This is
achieved primarily by adding a
`diffScan(long startTimestamp, long endTimestamp)`
method to the AbstractKuduScannerBuilder builder,
and then configuring the scan RPC to include the set fields.
The column projection of the scan will include an extra
“is_deleted” column when a diff scan is performed.
This column can be used like a standard boolean
column to detect if the row was deleted.
Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
---
M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.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
M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
M java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java
M src/kudu/client/client.proto
12 files changed, 349 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/12689/7
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
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: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] POC: [java] Add diff scan support
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12689 )
Change subject: POC: [java] Add diff scan support
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12689/1/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/12689/1/java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java@266
PS1, Line 266: * Requires that the ReadMode is READ_DIFF.
I saw READ_DIFF mentioned in Slack but I don't recall a conversation about it. Why do we need to introduce a new read mode to support diff scans? Why isn't it sufficient to add IS_DELETED and the start timestamp?
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 07 Mar 2019 19:13:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Add private diff scan support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12689 )
Change subject: [java] Add private diff scan support
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12689/6/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/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@190
PS6, Line 190: long startHT = HybridTimeUtil.clockTimestampToHTTimestamp(System.currentTimeMillis(),
> Any update here?
Done
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
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: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Tue, 19 Mar 2019 03:06:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Add private diff scan support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo, Greg Solovyev,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12689
to look at the new patch set (#13).
Change subject: [java] Add private diff scan support
......................................................................
[java] Add private diff scan support
Adds a private diff scan API to the java client. This is
achieved primarily by adding a
`diffScan(long startTimestamp, long endTimestamp)`
method to the AbstractKuduScannerBuilder builder,
and then configuring the scan RPC to include the set fields.
The column projection of the scan will include an extra
“is_deleted” column when a diff scan is performed.
This column can be used like a standard boolean
column to detect if the row was deleted.
Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
---
M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.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
M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
M java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java
M java/kudu-client/src/main/java/org/apache/kudu/util/TimestampUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java
M src/kudu/client/client.proto
13 files changed, 427 insertions(+), 53 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/12689/13
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
Gerrit-PatchSet: 13
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [java] Add private diff scan support
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12689 )
Change subject: [java] Add private diff scan support
......................................................................
Patch Set 6:
(30 comments)
http://gerrit.cloudera.org:8080/#/c/12689/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12689/6//COMMIT_MSG@10
PS6, Line 10: acheived
achieved
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
File java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java:
PS6:
"real" type doesn't add a whole lot of information, because "real" can have many definitions. It'd be great to use an adjective with a more concrete meaning. Maybe "pbType"? Or "wireType"? Is "physicalType" appropriate, or is that a misnomer too?
http://gerrit.cloudera.org:8080/#/c/12689/6/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/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java@266
PS6, Line 266: an encoded HT timestamp.
encoded HT timestamps
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java@268
PS6, Line 268: * Additionally sets the ReadMode to READ_AT_SNAPSHOT and isFaultTolerant to true.
I might generalize a bit and say something like "Sets the start/end timestamps as well as some other scan properties to support diff scans".
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java@277
PS6, Line 277: this.endTimestamp = endTimestamp;
Why not reuse htTimestamp for 'endTimestamp'? They're the same to the server.
http://gerrit.cloudera.org:8080/#/c/12689/6/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/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@323
PS6, Line 323: columns.addAll(table.getSchema().getColumns());
Why don't we need to strip the columns in this case?
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@325
PS6, Line 325: scan add
Nit: scan so add
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@355
PS6, Line 355: private static ColumnSchema getStrippedColumnSchema(ColumnSchema columnToClone) {
Seems like this should be a method on ColumnSchema; otherwise it's easy to forget to update it when adding new preservable fields to ColumnSchema (like realType).
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@1000
PS6, Line 1000: // if the mode is set to read on snapshot sent the snapshot timestamps.
send
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@1010
PS6, Line 1010: newBuilder.setSnapTimestamp(AsyncKuduScanner.this.getEndSnaphshotTimestamp());
Seems weird that we'd allow snapTimestamp to be set twice: once here and once on L1003.
Same goes for the scan token equivalent.
http://gerrit.cloudera.org:8080/#/c/12689/6/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/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@252
PS6, Line 252: // TODO: Move the logic that gets the indirectData index for String and Binary columns
: // into a separate private method and enforce only INT64 and UNIXTIME_MICROS here.
Is this a short-term or long-term TODO? Seems easy enough to address so might be worth doing short-term; this is on the hot path and it'd be cheap to address this new extra loop.
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@698
PS6, Line 698: buf.append(TimestampUtil.timestampToString(getLong(i)));
Kind of surprised this doesn't operate on a java.sql.Timestamp (and call getTimestamp(i)). Wouldn't that mean getLong() would only care about INT64, STRING, and BINARY (and after that new TODO is satisfied, just INT64)?
http://gerrit.cloudera.org:8080/#/c/12689/6/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:
PS6:
One thing that'd make the new test easier to read is if it separated its work into two distinct phases:
1. Generate a list of operations, including INSERTs, UPDATEs, and DELETEs.
2. Apply the list of operations to a session, injecting sleeps to let flushes occur.
This is the approach I took when I wrote MirroredDeltas (see tablet-test-util.h), and besides being clearer it also makes the "phase 1" stuff easily reusable by other tests.
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@60
PS6, Line 60: private static final int DIFF_FLUSH_SEC = 2;
Any particular reason not to make this 1? Seems like we should run the test as quickly as possible.
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@183
PS6, Line 183: Some Rows
Nit: some rows
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@190
PS6, Line 190: long startHT = HybridTimeUtil.clockTimestampToHTTimestamp(System.currentTimeMillis(),
Are you sure that startHT will have a value >= the HTs associated with the generated mutations?
Same question for the other HTs below.
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@199
PS6, Line 199: Operation.ChangeType
Can drop Operation prefix.
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@215
PS6, Line 215: // Pass through the scan token API to ensure serialization of tokens works too.
Can you also test the regular scan case? The code paths are a little different (different builder), right?
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@241
PS6, Line 241: assertTrue(result.getBoolean(IS_DELETED_COL_NAME));
Can we verify the inverse for INSERT/UPDATE?
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@244
PS6, Line 244: // This means the type was null, therefore the key was not found in the mutations map.
Not following this; how could a change type not be one of INSERT/UPDATE/DELETE? What does it mean to have a "null" type?
On that note, we should also test nullable columns and ensure that diff scans work properly when column values go from null to not-null and vice versa.
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@256
PS6, Line 256: * Generates a random permutations of rows.
It also sends the writes to the server; that's worth noting here too.
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@264
PS6, Line 264: KuduSession session = client.newSession();
Why not reuse the same session for all writes?
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@267
PS6, Line 267: ArrayList
Nit: List
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@297
PS6, Line 297: // insert -> delete|update
: // update -> delete|update
Nit: reorder update|delete to reflect the order of operations below.
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@302
PS6, Line 302: if (random.nextBoolean()) {
: op = table.newUpdate();
: } else {
: op = table.newDelete();
: }
Nit: combine via ternary.
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@315
PS6, Line 315: generator.randomizeRow(row, /* randomizeKeys */ false);
You can skip this if you're doing a DELETE. I mean, you merged that patch that makes this legal, but shouldn't the test do as little work as it can get away with?
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@325
PS6, Line 325: state.currentType == state.endType
This check happens too late to properly honor endType==INSERT.
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@337
PS6, Line 337: session.close();
Not necessary with APPLY_FLUSH_SYNC?
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@347
PS6, Line 347: if (resp.hasRowError()) {
: LOG.error("Could not insert row: " + resp.getRowError().getErrorStatus());
: }
: assertFalse(resp.hasRowError());
Can this be combined?
assertFalse("could not insert row: " + ..., resp.hasRowError())
You won't get the LOG, but is that important given that the test itself will fail?
Also in generateMutations.
http://gerrit.cloudera.org:8080/#/c/12689/6/src/kudu/client/client.proto
File src/kudu/client/client.proto:
http://gerrit.cloudera.org:8080/#/c/12689/6/src/kudu/client/client.proto@84
PS6, Line 84: O
typo. From 'snap_timestamp' it would seem.
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
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: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 14 Mar 2019 22:18:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Add private diff scan support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has removed a vote on this change.
Change subject: [java] Add private diff scan support
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
Gerrit-PatchSet: 11
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [java] Add private diff scan support
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12689 )
Change subject: [java] Add private diff scan support
......................................................................
Patch Set 8:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/12689/8/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
File java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java:
http://gerrit.cloudera.org:8080/#/c/12689/8/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@373
PS8, Line 373: * when serializing the
Sentence trails off.
http://gerrit.cloudera.org:8080/#/c/12689/8/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/12689/8/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@a251
PS8, Line 251:
Is there any concern that someone was previously abusing getLong() by using it on a STRING type? And now they won't be allowed to do that?
http://gerrit.cloudera.org:8080/#/c/12689/8/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@253
PS8, Line 253: return Bytes.getLong(this.rowData.getRawArray(),
: this.rowData.getRawOffset() +
: getCurrentRowDataOffsetForColumn(columnIndex));
This can chain to getIndirectDataOffset.
http://gerrit.cloudera.org:8080/#/c/12689/8/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@370
PS8, Line 370: long micros = Bytes.getLong(this.rowData.getRawArray(),
: this.rowData.getRawOffset() +
: getCurrentRowDataOffsetForColumn(columnIndex));
As can this.
http://gerrit.cloudera.org:8080/#/c/12689/8/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@497
PS8, Line 497: * @return the
Didn't fill this out?
http://gerrit.cloudera.org:8080/#/c/12689/8/java/kudu-client/src/main/java/org/apache/kudu/util/TimestampUtil.java
File java/kudu-client/src/main/java/org/apache/kudu/util/TimestampUtil.java:
http://gerrit.cloudera.org:8080/#/c/12689/8/java/kudu-client/src/main/java/org/apache/kudu/util/TimestampUtil.java@97
PS8, Line 97: public static String timestampToString(long micros) {
Kinda confusing that this method claims to convert a "timestamp" into a string when in fact it operates on a micros integer value. I mean, It'd be OK if we didn't have the other methods above for whom "timestamp" means java.sql.Timestamp.
http://gerrit.cloudera.org:8080/#/c/12689/6/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/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@190
PS6, Line 190: LOG.info("Before: {}", before);
> Are you sure that startHT will have a value >= the HTs associated with the
Any update here?
I think you could use KuduClient.getLastObservedTimestamp() for the lower bound, as it'll represent the latest timestamp used by the writes in applyOperations. Maybe need to +1 it though.
http://gerrit.cloudera.org:8080/#/c/12689/8/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/12689/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@266
PS8, Line 266: it's
Nit: its
http://gerrit.cloudera.org:8080/#/c/12689/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@269
PS8, Line 269: Map<Integer, ChangeType> results = new HashMap<>();
Worth noting that, except for logging, the test currently doesn't use these results at all, so unless you intend to do something with them in a subsequent patch, you could save a few LOC and not generate them.
http://gerrit.cloudera.org:8080/#/c/12689/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@336
PS8, Line 336: operations
operation
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
Gerrit-PatchSet: 8
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Mon, 18 Mar 2019 17:35:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Add private diff scan support
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12689 )
Change subject: [java] Add private diff scan support
......................................................................
Patch Set 9:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12689/8/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/12689/8/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@253
PS8, Line 253: return Bytes.getLong(this.rowData.getRawArray(),
: this.rowData.getRawOffset() +
: getCurrentRowDataOffsetForColumn(columnIndex));
> Even though the "code" is the same I don't call that function since this is
Why does it matter? If the code is the same, the result will be the same and we'll reduce LOC by a bit, and I'd add that this array offsetting is not trivial code either.
If you think it'll look confusing, a one-line comment should clarify while still allowing us to reuse the code.
http://gerrit.cloudera.org:8080/#/c/12689/8/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/12689/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@269
PS8, Line 269: Map<Integer, ChangeType> results = new HashMap<>();
> The return value is stored in the `mutations` variable and used to lookup a
Ah yes, of the three calls, one does use the return results.
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
Gerrit-PatchSet: 9
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Mon, 18 Mar 2019 18:03:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Add private diff scan support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo, Greg Solovyev,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12689
to look at the new patch set (#10).
Change subject: [java] Add private diff scan support
......................................................................
[java] Add private diff scan support
Adds a private diff scan API to the java client. This is
achieved primarily by adding a
`diffScan(long startTimestamp, long endTimestamp)`
method to the AbstractKuduScannerBuilder builder,
and then configuring the scan RPC to include the set fields.
The column projection of the scan will include an extra
“is_deleted” column when a diff scan is performed.
This column can be used like a standard boolean
column to detect if the row was deleted.
Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
---
M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.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
M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
M java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java
M java/kudu-client/src/main/java/org/apache/kudu/util/TimestampUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java
M src/kudu/client/client.proto
13 files changed, 428 insertions(+), 53 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/12689/10
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
Gerrit-PatchSet: 10
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [java] Add private diff scan support
Posted by "Greg Solovyev (Code Review)" <ge...@cloudera.org>.
Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/12689 )
Change subject: [java] Add private diff scan support
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12689/5/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/12689/5/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@54
PS5, Line 54: private static final Logger LOG = LoggerFactory.getLogger(TestScannerMultiTablet.class);
This also looks unused. Why is it needed?
http://gerrit.cloudera.org:8080/#/c/12689/5/java/kudu-client/src/test/java/org/apache/kudu/client/TestScannerMultiTablet.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestScannerMultiTablet.java:
http://gerrit.cloudera.org:8080/#/c/12689/5/java/kudu-client/src/test/java/org/apache/kudu/client/TestScannerMultiTablet.java@46
PS5, Line 46: private static final Logger LOG = LoggerFactory.getLogger(TestScannerMultiTablet.class);
This looks like an unused private member. Why is it needed here?
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
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: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 14 Mar 2019 20:59:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Add private diff scan support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12689 )
Change subject: [java] Add private diff scan support
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12689/8/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
File java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java:
http://gerrit.cloudera.org:8080/#/c/12689/8/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@373
PS8, Line 373: * when serializing the
> Sentence trails off.
Done
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
Gerrit-PatchSet: 8
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Mon, 18 Mar 2019 19:06:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Add private diff scan support
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12689 )
Change subject: [java] Add private diff scan support
......................................................................
Patch Set 15: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
Gerrit-PatchSet: 15
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Mon, 25 Mar 2019 18:00:10 +0000
Gerrit-HasComments: No
[kudu-CR] [java] Add private diff scan support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12689
to look at the new patch set (#5).
Change subject: [java] Add private diff scan support
......................................................................
[java] Add private diff scan support
Adds a private diff scan API to the java client. This is
acheived primarily by adding a
`diffScan(long startTimestamp, long endTimestamp)`
method to the AbstractKuduScannerBuilder builder,
and then configuring the scan RPC to include the set fields.
The column projection of the scan will include an extra
“is_deleted” column when a diff scan is performed.
This column can be used like a standard boolean
column to detect if the row was deleted.
Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
---
M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.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
M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
M java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScannerMultiTablet.java
M src/kudu/client/client.proto
13 files changed, 369 insertions(+), 30 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/12689/5
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
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)
[kudu-CR] [java] Add private diff scan support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12689 )
Change subject: [java] Add private diff scan support
......................................................................
Patch Set 6:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
File java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java:
PS6:
> Yeah I guess virtualType works.
I opted for wireType, since that won't ever be "wrong" if the motivation for its use changes in the future. I included a comment about virtual columns in the docs.
http://gerrit.cloudera.org:8080/#/c/12689/6/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/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@323
PS6, Line 323: columns.addAll(table.getSchema().getColumns());
> Yeah I think that's worth investigating:
Stripping was added in this commit: https://github.com/apache/kudu/commit/7d57b8b9204a0b170fc204cb7aaa98ad995cf635
It looks like its primary, maybe only, purpose is to remove the isKey property from a column to allow for key columns to be any order in a scan.
This seams like something the server side should allow regardless of isKey=true. I may introduce a patch to improve that some stripping columns can be removed. For now I will add a todo and update the function to strip the isKey only.
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@355
PS6, Line 355: private static ColumnSchema getStrippedColumnSchema(ColumnSchema columnToClone) {
> No clue; presumably if we pass along too much information back to the serve
See my comment above.
http://gerrit.cloudera.org:8080/#/c/12689/6/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/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@252
PS6, Line 252: // TODO: Move the logic that gets the indirectData index for String and Binary columns
: // into a separate private method and enforce only INT64 and UNIXTIME_MICROS here.
> It didn't feel appropriate for this patch. It can be done in a separate pat
I fixed this in this patch.
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@698
PS6, Line 698: buf.append(TimestampUtil.timestampToString(getLong(i)));
> Kind of surprised this doesn't operate on a java.sql.Timestamp (and call ge
Done
http://gerrit.cloudera.org:8080/#/c/12689/6/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:
PS6:
> One thing that'd make the new test easier to read is if it separated its wo
Done
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@244
PS6, Line 244: // This means the type was null, therefore the key was not found in the mutations map.
> I'd rephrase the comment to describe what this condition means w.r.t. the t
Done
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@325
PS6, Line 325: state.currentType == state.endType
> I see where the disconnect is: I assumed that endType==INSERT means the row
I moved this check to the top of the loop so that INSERTS can exist on their own too.
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@337
PS6, Line 337: session.close();
> When you said closable you set off my spidey sense: how do you feel about o
I can't because KuduSession doesn't implement the "AutoCloseable" interface. It's likely because it's close method returns "List<OperationResponse>".
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@347
PS6, Line 347: if (resp.hasRowError()) {
: LOG.error("Could not insert row: " + resp.getRowError().getErrorStatus());
: }
: assertFalse(resp.hasRowError());
> But don't assertion failures result in a displayed stack trace beginning wi
You mean something like this:
`assertFalse(resp.getRowError().getErrorStatus().toString(), resp.hasRowError());
`
The problem is the message is "aggressively" called regardless of failure and therefore results in a NPE because most often there isn't a row error.
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
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: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Mon, 18 Mar 2019 16:51:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Add private diff scan support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12689 )
Change subject: [java] Add private diff scan support
......................................................................
[java] Add private diff scan support
Adds a private diff scan API to the java client. This is
achieved primarily by adding a
`diffScan(long startTimestamp, long endTimestamp)`
method to the AbstractKuduScannerBuilder builder,
and then configuring the scan RPC to include the set fields.
The column projection of the scan will include an extra
`IS_DELETED` column when a diff scan is performed.
The value of the `IS_DELETED` virtual column can be
retrieved via `RowResult.isDeleted()`
Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Reviewed-on: http://gerrit.cloudera.org:8080/12689
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Mike Percy <mp...@apache.org>
---
M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
M java/kudu-client/src/main/java/org/apache/kudu/Schema.java
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.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
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
M java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java
M java/kudu-client/src/main/java/org/apache/kudu/util/TimestampUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java
M src/kudu/client/client.proto
15 files changed, 509 insertions(+), 55 deletions(-)
Approvals:
Kudu Jenkins: Verified
Adar Dembo: Looks good to me, approved
Mike Percy: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
Gerrit-PatchSet: 16
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [java] Add private diff scan support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12689 )
Change subject: [java] Add private diff scan support
......................................................................
Patch Set 11: Verified+1
Unrelated flaky that is resolved.
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
Gerrit-PatchSet: 11
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Mon, 18 Mar 2019 19:51:41 +0000
Gerrit-HasComments: No
[kudu-CR] [java] Add private diff scan support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo, Greg Solovyev,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12689
to look at the new patch set (#14).
Change subject: [java] Add private diff scan support
......................................................................
[java] Add private diff scan support
Adds a private diff scan API to the java client. This is
achieved primarily by adding a
`diffScan(long startTimestamp, long endTimestamp)`
method to the AbstractKuduScannerBuilder builder,
and then configuring the scan RPC to include the set fields.
The column projection of the scan will include an extra
`IS_DELETED` column when a diff scan is performed.
The value of the `IS_DELETED` virtual column can be
retrieved via `RowResult.isDeleted()`
Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
---
M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
M java/kudu-client/src/main/java/org/apache/kudu/Schema.java
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.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
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
M java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java
M java/kudu-client/src/main/java/org/apache/kudu/util/TimestampUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java
M src/kudu/client/client.proto
15 files changed, 510 insertions(+), 55 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/12689/14
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
Gerrit-PatchSet: 14
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] POC: [java] Add diff scan support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12689 )
Change subject: POC: [java] Add diff scan support
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12689/1/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/12689/1/java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java@266
PS1, Line 266: * Requires that the ReadMode is READ_DIFF.
> I saw READ_DIFF mentioned in Slack but I don't recall a conversation about
This is an outdated comment, I backed out my READ_DIFF mode. I did like being explicit, but this patch uses the existence of start/end time to handle diff scans.
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
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-Comment-Date: Thu, 07 Mar 2019 20:10:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Add private diff scan support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12689
to look at the new patch set (#3).
Change subject: [java] Add private diff scan support
......................................................................
[java] Add private diff scan support
Adds a private diff scan API to the java client. This is
acheived primarily by adding a
`diffScan(long startTimestamp, long endTimestamp)`
method to the AbstractKuduScannerBuilder builder,
and then configuring the scan RPC to include the set fields.
The column projection of the scan will include an extra
“is_deleted” column when a diff scan is performed.
This column can be used like a standard boolean
column to detect if the row was deleted.
Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
---
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.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
M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
M java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScannerMultiTablet.java
M src/kudu/client/client.proto
12 files changed, 332 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/12689/3
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
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)
[kudu-CR] [java] Add private diff scan support
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12689 )
Change subject: [java] Add private diff scan support
......................................................................
Patch Set 13: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
Gerrit-PatchSet: 13
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Tue, 19 Mar 2019 04:32:16 +0000
Gerrit-HasComments: No
[kudu-CR] [java] Add private diff scan support
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12689 )
Change subject: [java] Add private diff scan support
......................................................................
Patch Set 15: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
Gerrit-PatchSet: 15
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 22 Mar 2019 17:40:54 +0000
Gerrit-HasComments: No
[kudu-CR] [java] Add private diff scan support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo, Greg Solovyev,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12689
to look at the new patch set (#15).
Change subject: [java] Add private diff scan support
......................................................................
[java] Add private diff scan support
Adds a private diff scan API to the java client. This is
achieved primarily by adding a
`diffScan(long startTimestamp, long endTimestamp)`
method to the AbstractKuduScannerBuilder builder,
and then configuring the scan RPC to include the set fields.
The column projection of the scan will include an extra
`IS_DELETED` column when a diff scan is performed.
The value of the `IS_DELETED` virtual column can be
retrieved via `RowResult.isDeleted()`
Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
---
M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
M java/kudu-client/src/main/java/org/apache/kudu/Schema.java
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.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
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
M java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java
M java/kudu-client/src/main/java/org/apache/kudu/util/TimestampUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java
M src/kudu/client/client.proto
15 files changed, 509 insertions(+), 55 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/12689/15
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
Gerrit-PatchSet: 15
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [java] Add private diff scan support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12689 )
Change subject: [java] Add private diff scan support
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12689/5/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/12689/5/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@54
PS5, Line 54: private static final Logger LOG = LoggerFactory.getLogger(TestScannerMultiTablet.class);
> This also looks unused. Why is it needed?
This is used quite a bit in testDiffScan below.
http://gerrit.cloudera.org:8080/#/c/12689/5/java/kudu-client/src/test/java/org/apache/kudu/client/TestScannerMultiTablet.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestScannerMultiTablet.java:
http://gerrit.cloudera.org:8080/#/c/12689/5/java/kudu-client/src/test/java/org/apache/kudu/client/TestScannerMultiTablet.java@46
PS5, Line 46: private static final Logger LOG = LoggerFactory.getLogger(TestScannerMultiTablet.class);
> This looks like an unused private member. Why is it needed here?
Will remove, this was left over from earlier patches.
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
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: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 14 Mar 2019 21:04:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Add private diff scan support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12689 )
Change subject: [java] Add private diff scan support
......................................................................
Patch Set 8:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/12689/8/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/12689/8/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@a251
PS8, Line 251:
> Is there any concern that someone was previously abusing getLong() by using
I don't think so. Doing so would return the indirectData offset which isn't documented anywhere and is an internal detail. Additionally getting the indirectData offset is not useful to them because they can't use it to get a value from any public methods.
The only thing this would expose are bugs and data loss/corruption.
http://gerrit.cloudera.org:8080/#/c/12689/8/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@253
PS8, Line 253: return Bytes.getLong(this.rowData.getRawArray(),
: this.rowData.getRawOffset() +
: getCurrentRowDataOffsetForColumn(columnIndex));
> This can chain to getIndirectDataOffset.
Even though the "code" is the same I don't call that function since this is actually getting the long value and not the indirectData offset.
http://gerrit.cloudera.org:8080/#/c/12689/8/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@370
PS8, Line 370: long micros = Bytes.getLong(this.rowData.getRawArray(),
: this.rowData.getRawOffset() +
: getCurrentRowDataOffsetForColumn(columnIndex));
> As can this.
See above.
http://gerrit.cloudera.org:8080/#/c/12689/8/java/kudu-client/src/main/java/org/apache/kudu/util/TimestampUtil.java
File java/kudu-client/src/main/java/org/apache/kudu/util/TimestampUtil.java:
http://gerrit.cloudera.org:8080/#/c/12689/8/java/kudu-client/src/main/java/org/apache/kudu/util/TimestampUtil.java@97
PS8, Line 97: public static String timestampToString(long micros) {
> Kinda confusing that this method claims to convert a "timestamp" into a str
This is a hold over from when the only way to get a timestamp was via getLong on a UNIXTIME_MICROS column. In that context I think this makes sense.
http://gerrit.cloudera.org:8080/#/c/12689/8/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/12689/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@266
PS8, Line 266: it's
> Nit: its
Done
http://gerrit.cloudera.org:8080/#/c/12689/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@269
PS8, Line 269: Map<Integer, ChangeType> results = new HashMap<>();
> Worth noting that, except for logging, the test currently doesn't use these
The return value is stored in the `mutations` variable and used to lookup and compare against the scanned results.
http://gerrit.cloudera.org:8080/#/c/12689/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@336
PS8, Line 336: operations
> operation
Done
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
Gerrit-PatchSet: 8
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Mon, 18 Mar 2019 17:44:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP: [java] Add diff scan support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12689
to look at the new patch set (#2).
Change subject: WIP: [java] Add diff scan support
......................................................................
WIP: [java] Add diff scan support
Plumbs the diff scan support into the java client. This is
WIP patch that adds minimal support and a
rough test.
Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
---
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.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
M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
M java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScannerMultiTablet.java
M src/kudu/client/client.proto
12 files changed, 280 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/12689/2
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
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)
[kudu-CR] [java] Add private diff scan support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12689 )
Change subject: [java] Add private diff scan support
......................................................................
Patch Set 6:
(28 comments)
I addressed many of the reviews, or asked questions. I will follow up with the rest.
http://gerrit.cloudera.org:8080/#/c/12689/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12689/6//COMMIT_MSG@10
PS6, Line 10: acheived
> achieved
Done
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
File java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java:
PS6:
> "real" type doesn't add a whole lot of information, because "real" can have
I don't think physical type makes sense. That is primarily used server side when mapping a logical type to how it is stored on disk.
This is the type of the virtual column, maybe I should call it virtualType? wireType or pbType are technically the most "correct" given that is the type I will send on the wire.
http://gerrit.cloudera.org:8080/#/c/12689/6/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/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java@266
PS6, Line 266: an encoded HT timestamp.
> encoded HT timestamps
Done
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java@268
PS6, Line 268: * Additionally sets the ReadMode to READ_AT_SNAPSHOT and isFaultTolerant to true.
> I might generalize a bit and say something like "Sets the start/end timesta
Done
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java@277
PS6, Line 277: this.endTimestamp = endTimestamp;
> Why not reuse htTimestamp for 'endTimestamp'? They're the same to the serve
Done
http://gerrit.cloudera.org:8080/#/c/12689/6/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/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@323
PS6, Line 323: columns.addAll(table.getSchema().getColumns());
> Why don't we need to strip the columns in this case?
Not sure honestly...this was the existing behavior. I am not totally sure why we need to strip them at all.
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@325
PS6, Line 325: scan add
> Nit: scan so add
Done
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@355
PS6, Line 355: private static ColumnSchema getStrippedColumnSchema(ColumnSchema columnToClone) {
> Seems like this should be a method on ColumnSchema; otherwise it's easy to
Do you know why we need to strip the column schema?
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@1000
PS6, Line 1000: // if the mode is set to read on snapshot sent the snapshot timestamps.
> send
Done
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@1010
PS6, Line 1010: newBuilder.setSnapTimestamp(AsyncKuduScanner.this.getEndSnaphshotTimestamp());
> Seems weird that we'd allow snapTimestamp to be set twice: once here and on
Done
http://gerrit.cloudera.org:8080/#/c/12689/6/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/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@252
PS6, Line 252: // TODO: Move the logic that gets the indirectData index for String and Binary columns
: // into a separate private method and enforce only INT64 and UNIXTIME_MICROS here.
> Is this a short-term or long-term TODO? Seems easy enough to address so mig
It didn't feel appropriate for this patch. It can be done in a separate patch. I only left the improved checkType behavior because I added it at one point when I had a IS_DELETED type.
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@698
PS6, Line 698: buf.append(TimestampUtil.timestampToString(getLong(i)));
> Kind of surprised this doesn't operate on a java.sql.Timestamp (and call ge
This section could use the timestamp. But we still need to support calling getLong for UNIXTIME_MICROS columns for backwards compatibility. We have the same pattern in PartialRow.
http://gerrit.cloudera.org:8080/#/c/12689/6/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/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@60
PS6, Line 60: private static final int DIFF_FLUSH_SEC = 2;
> Any particular reason not to make this 1? Seems like we should run the test
Done
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@183
PS6, Line 183: Some Rows
> Nit: some rows
Done
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@199
PS6, Line 199: Operation.ChangeType
> Can drop Operation prefix.
Done
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@215
PS6, Line 215: // Pass through the scan token API to ensure serialization of tokens works too.
> Can you also test the regular scan case? The code paths are a little differ
When the scan token is deserialized it uses KuduScannerBuilder. All of them share most of the implementation via AbstractKuduScannerBuilder.
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@241
PS6, Line 241: assertTrue(result.getBoolean(IS_DELETED_COL_NAME));
> Can we verify the inverse for INSERT/UPDATE?
Done
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@244
PS6, Line 244: // This means the type was null, therefore the key was not found in the mutations map.
> Not following this; how could a change type not be one of INSERT/UPDATE/DEL
It means a key was in the results of the scanner, but not in the original mutations map. That is why I increment resultExtra and fail the test if this happens.
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@256
PS6, Line 256: * Generates a random permutations of rows.
> It also sends the writes to the server; that's worth noting here too.
Done
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@264
PS6, Line 264: KuduSession session = client.newSession();
> Why not reuse the same session for all writes?
I can, this just seemed like a convenient place to contain a "session". Is there a reason to use the same sessions vs multiple?
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@267
PS6, Line 267: ArrayList
> Nit: List
Done
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@297
PS6, Line 297: // insert -> delete|update
: // update -> delete|update
> Nit: reorder update|delete to reflect the order of operations below.
Done
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@302
PS6, Line 302: if (random.nextBoolean()) {
: op = table.newUpdate();
: } else {
: op = table.newDelete();
: }
> Nit: combine via ternary.
Done
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@315
PS6, Line 315: generator.randomizeRow(row, /* randomizeKeys */ false);
> You can skip this if you're doing a DELETE. I mean, you merged that patch t
I think it's nice to be able to generate the row with the same code for all OP types. Otherwise I need to move this up to the if clause above and duplicate it.
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@325
PS6, Line 325: state.currentType == state.endType
> This check happens too late to properly honor endType==INSERT.
INSERT will still be a valid endType, some mutations will occur on the original inserted row though.
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@337
PS6, Line 337: session.close();
> Not necessary with APPLY_FLUSH_SYNC?
I still like to close sessions when I am done with them. I think it's a good practice for anything closable.
http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@347
PS6, Line 347: if (resp.hasRowError()) {
: LOG.error("Could not insert row: " + resp.getRowError().getErrorStatus());
: }
: assertFalse(resp.hasRowError());
> Can this be combined?
I broke it out so that the error would be printed when the test fails, otherwise the first step to debugging is adding the log back in.
http://gerrit.cloudera.org:8080/#/c/12689/6/src/kudu/client/client.proto
File src/kudu/client/client.proto:
http://gerrit.cloudera.org:8080/#/c/12689/6/src/kudu/client/client.proto@84
PS6, Line 84: O
> typo. From 'snap_timestamp' it would seem.
Done
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
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: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Sat, 16 Mar 2019 03:59:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Add private diff scan support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo, Greg Solovyev,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12689
to look at the new patch set (#6).
Change subject: [java] Add private diff scan support
......................................................................
[java] Add private diff scan support
Adds a private diff scan API to the java client. This is
acheived primarily by adding a
`diffScan(long startTimestamp, long endTimestamp)`
method to the AbstractKuduScannerBuilder builder,
and then configuring the scan RPC to include the set fields.
The column projection of the scan will include an extra
“is_deleted” column when a diff scan is performed.
This column can be used like a standard boolean
column to detect if the row was deleted.
Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
---
M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.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
M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
M java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java
M src/kudu/client/client.proto
12 files changed, 365 insertions(+), 30 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/12689/6
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
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: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [java] Add private diff scan support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12689 )
Change subject: [java] Add private diff scan support
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12689/8/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/12689/8/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@253
PS8, Line 253: return Bytes.getLong(this.rowData.getRawArray(),
: this.rowData.getRawOffset() +
: getCurrentRowDataOffsetForColumn(columnIndex));
> Why does it matter? If the code is the same, the result will be the same an
I renamed the method and reused it.
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
Gerrit-PatchSet: 8
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Mon, 18 Mar 2019 19:05:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Add private diff scan support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo, Greg Solovyev,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12689
to look at the new patch set (#9).
Change subject: [java] Add private diff scan support
......................................................................
[java] Add private diff scan support
Adds a private diff scan API to the java client. This is
achieved primarily by adding a
`diffScan(long startTimestamp, long endTimestamp)`
method to the AbstractKuduScannerBuilder builder,
and then configuring the scan RPC to include the set fields.
The column projection of the scan will include an extra
“is_deleted” column when a diff scan is performed.
This column can be used like a standard boolean
column to detect if the row was deleted.
Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
---
M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.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
M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
M java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java
M java/kudu-client/src/main/java/org/apache/kudu/util/TimestampUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java
M src/kudu/client/client.proto
13 files changed, 424 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/12689/9
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
Gerrit-PatchSet: 9
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [java] Add private diff scan support
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12689 )
Change subject: [java] Add private diff scan support
......................................................................
Patch Set 11: Code-Review+1
I'll +2 once we resolve that outstanding issue regarding HT timestamp selection.
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
Gerrit-PatchSet: 11
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Mon, 18 Mar 2019 22:11:57 +0000
Gerrit-HasComments: No
[kudu-CR] [java] Add private diff scan support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo, Greg Solovyev,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12689
to look at the new patch set (#11).
Change subject: [java] Add private diff scan support
......................................................................
[java] Add private diff scan support
Adds a private diff scan API to the java client. This is
achieved primarily by adding a
`diffScan(long startTimestamp, long endTimestamp)`
method to the AbstractKuduScannerBuilder builder,
and then configuring the scan RPC to include the set fields.
The column projection of the scan will include an extra
“is_deleted” column when a diff scan is performed.
This column can be used like a standard boolean
column to detect if the row was deleted.
Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
---
M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.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
M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
M java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java
M java/kudu-client/src/main/java/org/apache/kudu/util/TimestampUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java
M src/kudu/client/client.proto
13 files changed, 428 insertions(+), 53 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/12689/11
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
Gerrit-PatchSet: 11
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [java] Add private diff scan support
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12689
to look at the new patch set (#4).
Change subject: [java] Add private diff scan support
......................................................................
[java] Add private diff scan support
Adds a private diff scan API to the java client. This is
acheived primarily by adding a
`diffScan(long startTimestamp, long endTimestamp)`
method to the AbstractKuduScannerBuilder builder,
and then configuring the scan RPC to include the set fields.
The column projection of the scan will include an extra
“is_deleted” column when a diff scan is performed.
This column can be used like a standard boolean
column to detect if the row was deleted.
Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
---
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.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
M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
M java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScannerMultiTablet.java
M src/kudu/client/client.proto
12 files changed, 334 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/12689/4
--
To view, visit http://gerrit.cloudera.org:8080/12689
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964
Gerrit-Change-Number: 12689
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)