You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2016/06/15 17:43:03 UTC
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Hello Jean-Daniel Cryans, Adar Dembo,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/3388
to review the following change.
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
KUDU-1309: [java client] support tables with non-covering partition-key ranges
This commit adds support for accessing and creating tables with non-covering
range partitions to the Java client. The only public interface introduced is the
CreateTableOptions.addRangeBound method. Otherwise, all the changes are
necessary for reading or writing to tables with range partition gaps. Right now
if a client attempts to write to a non-covered range, a NotFound status is
returned, which matches the C++ client. JD pointed out that this makes it
impossible for applications to distinguish between failed updates because the
row doesn't exist, and failed updates because the partition range doesn't exist.
I plan to fix this by introducing a new status code and fixing both clients in a
follow up commit.
Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
---
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
M java/kudu-client/src/main/java/org/kududb/client/Batch.java
M java/kudu-client/src/main/java/org/kududb/client/Bytes.java
M java/kudu-client/src/main/java/org/kududb/client/CreateTableOptions.java
M java/kudu-client/src/main/java/org/kududb/client/CreateTableRequest.java
M java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java
M java/kudu-client/src/main/java/org/kududb/client/KuduScanner.java
A java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeCache.java
A java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeException.java
M java/kudu-client/src/main/java/org/kududb/client/Operation.java
M java/kudu-client/src/main/java/org/kududb/client/Partition.java
M java/kudu-client/src/main/java/org/kududb/client/RowResultIterator.java
M java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/kududb/client/TestFlexiblePartitioning.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduSession.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduTable.java
19 files changed, 698 insertions(+), 99 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/3388/1
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3388
to look at the new patch set (#4).
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
KUDU-1309: [java client] support tables with non-covering partition-key ranges
This commit adds support for accessing and creating tables with non-covering
range partitions to the Java client. The only public interface introduced is the
CreateTableOptions.addRangeBound method. Otherwise, all the changes are
necessary for reading or writing to tables with range partition gaps. Right now
if a client attempts to write to a non-covered range, a NotFound status is
returned, which matches the C++ client. JD pointed out that this makes it
impossible for applications to distinguish between failed updates because the
row doesn't exist, and failed updates because the partition range doesn't exist.
I plan to fix this by introducing a new status code and fixing both clients in a
follow up commit.
Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
---
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
M java/kudu-client/src/main/java/org/kududb/client/Batch.java
M java/kudu-client/src/main/java/org/kududb/client/BatchResponse.java
M java/kudu-client/src/main/java/org/kududb/client/Bytes.java
M java/kudu-client/src/main/java/org/kududb/client/CreateTableOptions.java
M java/kudu-client/src/main/java/org/kududb/client/CreateTableRequest.java
M java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java
M java/kudu-client/src/main/java/org/kududb/client/KuduRpcResponse.java
A java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeCache.java
A java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeException.java
M java/kudu-client/src/main/java/org/kududb/client/Operation.java
M java/kudu-client/src/main/java/org/kududb/client/OperationResponse.java
M java/kudu-client/src/main/java/org/kududb/client/Partition.java
M java/kudu-client/src/main/java/org/kududb/client/RowError.java
M java/kudu-client/src/main/java/org/kududb/client/RowResultIterator.java
M java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/kududb/client/TestFlexiblePartitioning.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduSession.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduTable.java
22 files changed, 830 insertions(+), 138 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/3388/4
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
Patch Set 3:
Build Started http://104.196.14.100/job/kudu-gerrit/1975/
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
Patch Set 1:
Build Started http://104.196.14.100/job/kudu-gerrit/1831/
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3388
to look at the new patch set (#8).
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
KUDU-1309: [java client] support tables with non-covering partition-key ranges
This commit adds support for accessing and creating tables with non-covering
range partitions to the Java client. The only public interface introduced is the
CreateTableOptions.addRangeBound method. Otherwise, all the changes are
necessary for reading or writing to tables with range partition gaps. Right now
if a client attempts to write to a non-covered range, a NotFound status is
returned, which matches the C++ client. JD pointed out that this makes it
impossible for applications to distinguish between failed updates because the
row doesn't exist, and failed updates because the partition range doesn't exist.
I plan to fix this by introducing a new status code and fixing both clients in a
follow up commit.
Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
---
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
M java/kudu-client/src/main/java/org/kududb/client/Batch.java
M java/kudu-client/src/main/java/org/kududb/client/BatchResponse.java
M java/kudu-client/src/main/java/org/kududb/client/Bytes.java
M java/kudu-client/src/main/java/org/kududb/client/CreateTableOptions.java
M java/kudu-client/src/main/java/org/kududb/client/CreateTableRequest.java
M java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java
M java/kudu-client/src/main/java/org/kududb/client/KuduRpcResponse.java
A java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeCache.java
A java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeException.java
M java/kudu-client/src/main/java/org/kududb/client/Operation.java
M java/kudu-client/src/main/java/org/kududb/client/OperationResponse.java
M java/kudu-client/src/main/java/org/kududb/client/Partition.java
M java/kudu-client/src/main/java/org/kududb/client/RowError.java
M java/kudu-client/src/main/java/org/kududb/client/RowResultIterator.java
M java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/kududb/client/TestFlexiblePartitioning.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduSession.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduTable.java
22 files changed, 839 insertions(+), 138 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/3388/8
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
Patch Set 2:
Build Started http://104.196.14.100/job/kudu-gerrit/1835/
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
Patch Set 8:
Build Started http://104.196.14.100/job/kudu-gerrit/2184/
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
Patch Set 1:
(23 comments)
http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java:
Line 154: * Map of table ID to non-covered range cache.
> Can you describe the lifecycle of items the get into this cache? Going thro
Done
Line 684: // TODO: should this throw instead?
> Unit test it and see what happens? :)
Woops, these were leftover from before NonCoveredRangeException. Ideally NonCoveredRangeException wouldn't exist and it would just be a normal KuduException with a status that indicates a non covered range, but until your error handling cleanup, this will have to do.
Line 1371: if (locations.isEmpty()) {
> How does this happen? Remove all the tablets?
Yah, currently this never happens since you can't create an empty table, and you can't remove ranges. Eventually it will be possible. I've expanded the comment a bit, since the logic about why it indicates an empty table is pretty subtle.
PS1, Line 1376: then
> the*
Done
http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduScanner.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduScanner.java:
Line 443: //LOG.info("Scan.open is returning rows: " + resp.data.getNumRows());
> woops, we've been carrying this for a while. Please remove? :)
Done
http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java:
PS1, Line 121: AUTO_FLUSH_BACKGROUND
> I thought it was only for manual flush?
Done
http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java
File java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java:
PS1, Line 84: .
> nit: don't end with period here.
Done
Line 86: boolean partitionKeyOrNext() {
> I was expecting this to be used for scans, but it's not overrode anywhere?
It was overriden in the async kudu scanner, but I ended up simplifying this out by just using NonCoveredRangeException everywhere (and the scanner specifically recovers from it).
http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/main/java/org/kududb/client/KuduScanner.java
File java/kudu-client/src/main/java/org/kududb/client/KuduScanner.java:
Line 57: RowResultIterator i = d.join(asyncScanner.scanRequestTimeout);
> Why was this this necessary?
woops, holdover from debugging.
http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeCache.java
File java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeCache.java:
Line 1: package org.kududb.client;
> License.
Done
PS1, Line 23: AsyncKuduClient
> Using that on purpose?
Done
Line 26: private final ConcurrentNavigableMap<byte[], byte[]> nonCoveredRanges =
> Another case where we don't seem to invalidate the cache. In fact, we don't
Added a note to the javadoc. This will come later, but this class gives us a good location to put the logic.
Line 30: *
> Finish the javadoc.
Done
Line 45: public void addNonCoveredRange(byte[] startPartitionKey, byte[] endPartitionKey) {
> Add javadoc.
Done
http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeException.java
File java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeException.java:
Line 1: package org.kududb.client;
> License.
Done
Line 6: public class NonCoveredRangeException extends KuduException {
> Missing interface audience.
Done
http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/main/java/org/kududb/client/RowResultIterator.java
File java/kudu-client/src/main/java/org/kududb/client/RowResultIterator.java:
Line 79: return new RowResultIterator(0, null, null, null, null);
> Should we consider keeping a single static instance? I don't think it's pos
Done
http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java
File java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java:
Line 239: public static CreateTableOptions getBasicTableOptionsWithNonCoveredRange() {
> Put in the javadoc a quick representation of what the table ends up looking
Done
http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/test/java/org/kududb/client/TestFlexiblePartitioning.java
File java/kudu-client/src/test/java/org/kududb/client/TestFlexiblePartitioning.java:
Line 249: @Test
> Add timeouts here and below.
Done
http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/test/java/org/kududb/client/TestKuduClient.java
File java/kudu-client/src/test/java/org/kududb/client/TestKuduClient.java:
PS1, Line 412: .
> remove period.
Done
Line 467: assertEquals(0, countRowsForTestScanNonCoveredTable(table, null, -1));
> I'm not sure looking at this if we're expecting 0 rows because none are ins
This tests inserts a value into every row in the covered range. This particular instance is only scanning non-covered partition space, so there are no results.
http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/test/java/org/kududb/client/TestKuduSession.java
File java/kudu-client/src/test/java/org/kududb/client/TestKuduSession.java:
Line 221: String tableName = TABLE_NAME_PREFIX + "-manual-flush-non-covered-range";
> Seems like those first lines can be refactored out in all tests.
Done
Line 290:
> No negative testing?
These tests are only testing what happens when inserting into non-covered ranges. The tests which scan non-covered ranges all insert into covered ranges within tables with non-covered ranges.
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/3388/4/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java:
Line 327: operation.callback(response);
> Dan, I think you missed this discussion. Sounds like there's a missing chec
Done
http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeCache.java
File java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeCache.java:
Line 84: @Override
> I think you missed this.
fixed.
Line 89: for (Map.Entry<byte[], byte[]> range : nonCoveredRanges.entrySet()) {
> Think you missed this.
yah I'm not crazy about Joiner unless there is already a List<String>.
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Jean-Daniel Cryans, Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3388
to look at the new patch set (#3).
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
KUDU-1309: [java client] support tables with non-covering partition-key ranges
This commit adds support for accessing and creating tables with non-covering
range partitions to the Java client. The only public interface introduced is the
CreateTableOptions.addRangeBound method. Otherwise, all the changes are
necessary for reading or writing to tables with range partition gaps. Right now
if a client attempts to write to a non-covered range, a NotFound status is
returned, which matches the C++ client. JD pointed out that this makes it
impossible for applications to distinguish between failed updates because the
row doesn't exist, and failed updates because the partition range doesn't exist.
I plan to fix this by introducing a new status code and fixing both clients in a
follow up commit.
Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
---
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
M java/kudu-client/src/main/java/org/kududb/client/Batch.java
M java/kudu-client/src/main/java/org/kududb/client/BatchResponse.java
M java/kudu-client/src/main/java/org/kududb/client/Bytes.java
M java/kudu-client/src/main/java/org/kududb/client/CreateTableOptions.java
M java/kudu-client/src/main/java/org/kududb/client/CreateTableRequest.java
M java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java
M java/kudu-client/src/main/java/org/kududb/client/KuduRpcResponse.java
A java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeCache.java
A java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeException.java
M java/kudu-client/src/main/java/org/kududb/client/Operation.java
M java/kudu-client/src/main/java/org/kududb/client/OperationResponse.java
M java/kudu-client/src/main/java/org/kududb/client/Partition.java
M java/kudu-client/src/main/java/org/kududb/client/RowError.java
M java/kudu-client/src/main/java/org/kududb/client/RowResultIterator.java
M java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/kududb/client/TestFlexiblePartitioning.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduSession.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduTable.java
22 files changed, 830 insertions(+), 138 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/3388/3
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
Patch Set 1:
(23 comments)
http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java:
Line 154: * Map of table ID to non-covered range cache.
Can you describe the lifecycle of items the get into this cache? Going through the code it doesn't seem like we invalidate it.
Line 684: // TODO: should this throw instead?
Unit test it and see what happens? :)
Line 1371: if (locations.isEmpty()) {
How does this happen? Remove all the tablets?
PS1, Line 1376: then
the*
http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduScanner.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduScanner.java:
Line 443: //LOG.info("Scan.open is returning rows: " + resp.data.getNumRows());
woops, we've been carrying this for a while. Please remove? :)
http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java:
PS1, Line 121: AUTO_FLUSH_BACKGROUND
I thought it was only for manual flush?
http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java
File java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java:
PS1, Line 84: .
nit: don't end with period here.
Line 86: boolean partitionKeyOrNext() {
I was expecting this to be used for scans, but it's not overrode anywhere?
http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/main/java/org/kududb/client/KuduScanner.java
File java/kudu-client/src/main/java/org/kududb/client/KuduScanner.java:
Line 57: RowResultIterator i = d.join(asyncScanner.scanRequestTimeout);
Why was this this necessary?
http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeCache.java
File java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeCache.java:
Line 1: package org.kududb.client;
License.
PS1, Line 23: AsyncKuduClient
Using that on purpose?
Line 26: private final ConcurrentNavigableMap<byte[], byte[]> nonCoveredRanges =
Another case where we don't seem to invalidate the cache. In fact, we don't seem to be able to override it either.
Line 30: *
Finish the javadoc.
Line 45: public void addNonCoveredRange(byte[] startPartitionKey, byte[] endPartitionKey) {
Add javadoc.
http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeException.java
File java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeException.java:
Line 1: package org.kududb.client;
License.
Line 6: public class NonCoveredRangeException extends KuduException {
Missing interface audience.
http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/main/java/org/kududb/client/RowResultIterator.java
File java/kudu-client/src/main/java/org/kududb/client/RowResultIterator.java:
Line 79: return new RowResultIterator(0, null, null, null, null);
Should we consider keeping a single static instance? I don't think it's possible to modify them once they return null, so they're safe to reuse?
http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java
File java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java:
Line 239: public static CreateTableOptions getBasicTableOptionsWithNonCoveredRange() {
Put in the javadoc a quick representation of what the table ends up looking like.
http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/test/java/org/kududb/client/TestFlexiblePartitioning.java
File java/kudu-client/src/test/java/org/kududb/client/TestFlexiblePartitioning.java:
Line 249: @Test
Add timeouts here and below.
http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/test/java/org/kududb/client/TestKuduClient.java
File java/kudu-client/src/test/java/org/kududb/client/TestKuduClient.java:
PS1, Line 412: .
remove period.
Line 467: assertEquals(0, countRowsForTestScanNonCoveredTable(table, null, -1));
I'm not sure looking at this if we're expecting 0 rows because none are inserted in that range or if we're in non-covered space. Also, not sure if you're testing a scan that starts in non-covered territory.
http://gerrit.cloudera.org:8080/#/c/3388/1/java/kudu-client/src/test/java/org/kududb/client/TestKuduSession.java
File java/kudu-client/src/test/java/org/kududb/client/TestKuduSession.java:
Line 221: String tableName = TABLE_NAME_PREFIX + "-manual-flush-non-covered-range";
Seems like those first lines can be refactored out in all tests.
Line 290:
No negative testing?
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3388/4/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java:
Line 327: operation.callback(response);
> Does this invoke the user callback eventually? If so, why do it before addi
It will do it right here, since it's the operation's callback were invoking.
Good question regarding the ordering. Actually, aren't we missing a check on AUTO_FLUSH_BACKGROUND before adding it to the error collector here? But for the ordering it doesn't really matter IMO, the user either rely on the callback or the collector, not both.
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
Patch Set 8: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
Patch Set 5:
Build Started http://104.196.14.100/job/kudu-gerrit/2091/
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has uploaded a new patch set (#2).
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
KUDU-1309: [java client] support tables with non-covering partition-key ranges
This commit adds support for accessing and creating tables with non-covering
range partitions to the Java client. The only public interface introduced is the
CreateTableOptions.addRangeBound method. Otherwise, all the changes are
necessary for reading or writing to tables with range partition gaps. Right now
if a client attempts to write to a non-covered range, a NotFound status is
returned, which matches the C++ client. JD pointed out that this makes it
impossible for applications to distinguish between failed updates because the
row doesn't exist, and failed updates because the partition range doesn't exist.
I plan to fix this by introducing a new status code and fixing both clients in a
follow up commit.
Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
---
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
M java/kudu-client/src/main/java/org/kududb/client/Batch.java
M java/kudu-client/src/main/java/org/kududb/client/Bytes.java
M java/kudu-client/src/main/java/org/kududb/client/CreateTableOptions.java
M java/kudu-client/src/main/java/org/kududb/client/CreateTableRequest.java
M java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java
A java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeCache.java
A java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeException.java
M java/kudu-client/src/main/java/org/kududb/client/Operation.java
M java/kudu-client/src/main/java/org/kududb/client/Partition.java
M java/kudu-client/src/main/java/org/kududb/client/RowResultIterator.java
M java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/kududb/client/TestFlexiblePartitioning.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduSession.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduTable.java
18 files changed, 752 insertions(+), 112 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/3388/2
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
Patch Set 2:
(32 comments)
http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java:
PS2, Line 139: This map
: * is always the first to be updated, because that's the map from
: * which all the lookups are done in the fast-path of the requests
: * that need to locate a tablet. The second map to be updated is
: * tablet2client, because it comes second in the fast-path
: * of every requests that need to locate a tablet. The third map
: * is only used to handle TabletServer disconnections gracefully.
> Nit: reading this again (I reviewed it eons ago), I think this part of the
I moved the non covered range cache out from under this comment since it really doesn't fit (it's not the same data indexed differently). I agree that this comment is not great, but it should be fixed with a proper meta cache.
Line 155: * Currently once a non-covered range is added to the cache, it is never
> Nit: prefix this part of the comment with "TODO:" since it is expected to c
Done
Line 576: Deferred<AsyncKuduScanner.Response> openScanner(final AsyncKuduScanner scanner) {
> True, this can be removed.
Done
Line 685: // Otherwise fall through to below where a GetTableLocations lookup will occur
> Nit: end with a period.
Done
Line 1148: Pair<byte[], byte[]> nonCoveredRange = getNonCoveredRange(table.getTableId(), key);
> Nit: if you want, you can call getTableId() once outside of the loop, store
Done
PS2, Line 1355: NonCoveredRangeCache nonCoveredRanges = nonCoveredRangeCaches.get(tableId);
: if (nonCoveredRanges == null) {
: nonCoveredRanges = new NonCoveredRangeCache();
: NonCoveredRangeCache oldCache = nonCoveredRangeCaches.putIfAbsent(tableId, nonCoveredRanges);
: if (oldCache != null) {
: nonCoveredRanges = oldCache;
: }
: }
> How about just:
putIfAbsent returns null if the entry was absent.
Line 1378: nonCoveredRanges.addNonCoveredRange(EMPTY_ARRAY, firstStartKey);
> Don't we need all of these calls to addNonCoveredRange() to be atomic? With
It may contain stale entries, but that needs to be handled once add/drop partition is introduced anyway.
http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduScanner.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduScanner.java:
Line 474: invalidate();
> Nit: invalidate() is called in both branches, can we pull it out?
Done
http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java:
Line 127: private final List<OperationResponse> failedLookups = new ArrayList<>();
> I think you can use a CopyOnWriteArrayList here and omit the use of a lock
I think in this case (where we are incrementally building up the list), a mutex is the best answer. Doing the checked put shuffle with an AtomicReference<List> is going to be hard and involve spinning + rebuilding the list per spin. COWAL isn't even lock free: http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/8u40-b25/java/util/concurrent/CopyOnWriteArrayList.java#433
Line 303: Callback<Deferred<List<OperationResponse>>, ArrayList<BatchResponse>> {
> Nit: not related to your change, but why is ArrayList the second type and n
Done
Line 306: batchResponses = batchResponses == null ? new ArrayList<BatchResponse>() : batchResponses;
> Is a null batchResponses actually possible? How?
I'm not sure, the code already handled the null case. When I comment out this line the tests still pass. JD, do you know what the deal is?
PS2, Line 308: // flushTablet() can return null when a tablet we wanted to flush was already flushed. Those
: // nulls along with BatchResponses are then put in a list by Deferred.group(). We first need
: // to filter the nulls out.
> OMG.
Agreed. Refactored.
Line 320: List<OperationResponse> responsesList = new ArrayList<>(size);
> If we're going to go through the trouble of precomputing the size, we ought
I thought about this, but the only way to figure out how many failed lookups there are is to take out the lock. Since there really shouldn't be failed lookups I decided not to slow down the normal path. I'm not too concerned about making the case where there are slow lookups be slow.
Line 397: if (tablet == null) {
> Why don't we perform this check to AUTO_FLUSH_SYNC on L377-384? Seems usefu
The first step of sendRpcToTablet (again, despite the name), is looking up the tablet to send it to based on the Rpc's partition key. So the check is done before an RPC is made, but it's done in sendRpcToTablet.
All that being said, I'm not sure why we have to do it here. Or for that matter, why AsyncKuduSession is tracking the lookups in flight at all. Seems to me all that should happen for a batched apply is that a new GetTableLocations should be kicked off if the tablet isn't known for the RPC, and otherwise just add it to the batch. On flush, it can just be sent through sentRpcToTablet. If the probe has returned in time then it will be faster, otherwise it goes through the lookup process again.
PS2, Line 399: new RowError(Status.NotFound("Write to a non-covered range partition"),
: operation, null);
> Could you add a two-arg constructor for this case?
Done
PS2, Line 401: new OperationResponse(0, null, 0, operation, rowError);
> Likewise, perhaps add a new constructor for this?
OperationResponse already relies on setting null params to the point where it's entwined in the control flow of Operation::deserialize
PS2, Line 416: if (lookupsDone != null && operationsInLookup.isEmpty()) {
: lookupsDoneCopy = lookupsDone;
: lookupsDone = null;
: }
> Copied from addToBuffer(), right? Can it be decomposed into a helper with a
refactored into oblivion
Line 422: operation.callback(response);
> Why do we do this?
this completes the individual operation response returned from #apply.
http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/main/java/org/kududb/client/CreateTableOptions.java
File java/kudu-client/src/main/java/org/kududb/client/CreateTableOptions.java:
Line 57: * The table creator takes ownership of the rows. If either row is empty, then
> Is this copied from the C++ client? What does 'ownership' mean here? In any
Done
http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java
File java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java:
Line 77: return null;
> Why return null instead of throwing some sort of "unsupported operation" ex
It's not really an exceptional circumstance, every master RPC falls in this category, for instance.
http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeCache.java
File java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeCache.java:
Line 39: final class NonCoveredRangeCache {
> Nit: why 'final'? The class is package-private, so it seems like the only c
Done
Line 40: public static final Logger LOG = LoggerFactory.getLogger(NonCoveredRangeCache.class);
> Nit: can be private
Done
Line 59: if (COMPARATOR.compare(partitionKey, range.getKey()) >= 0 &&
> Isn't this first comparison implied by the floorEntry() call?
Done
Line 61: return new Pair<>(range.getKey(), range.getValue());
> Why can't we return the range as-is? Why do we need to make a copy?
Done
Line 77: if (nonCoveredRanges.put(startPartitionKey, endPartitionKey) == null) {
> May want to comment on the concurrent behavior (i.e. multiple callers in di
Done
Line 84: public String toString() {
> Looks like this will return an inconsistent string representation if nonCov
if concurrent updates happen to the entries the iteration is undefined, so I'm not sure there is much to be done.
http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeException.java
File java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeException.java:
Line 26: @InterfaceStability.Unstable
> Why not evolving?
evolving is at a higher level of stability than unstable.
http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java
File java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java:
PS2, Line 243: * * [ 0, 50)
: * * [ 50, 100)
: * * [200, 300)
> Nit: what does the extra '*' mean?
Done
http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/test/java/org/kududb/client/TestFlexiblePartitioning.java
File java/kudu-client/src/test/java/org/kududb/client/TestFlexiblePartitioning.java:
Line 104: KuduScanner s = syncClient.newScannerBuilder(table).scanRequestTimeout(9999999).build();
> That's an odd choice of timeout. Why? And if you're looking for a large num
woops, left over from debugging.
http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/test/java/org/kududb/client/TestKuduClient.java
File java/kudu-client/src/test/java/org/kududb/client/TestKuduClient.java:
Line 457: if (key == 0) session.flush();
> Why flush after the very first operation? What's the significance of that?
not sure; removed.
http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/test/java/org/kududb/client/TestKuduSession.java
File java/kudu-client/src/test/java/org/kududb/client/TestKuduSession.java:
Line 35: tableName = TestKuduClient.class.getName() + "-" + System.currentTimeMillis();
> FWIW, even though this is guaranteed to run with every test, if a test fini
Done
Line 221: createOptions.setNumReplicas(1);
> Why do the new tests use only one replica, but the old ones don't?
Replicas aren't important for these particular tests. Additionally, they create more tablets per table (the other tables have a single tablet).
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3388
to look at the new patch set (#7).
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
KUDU-1309: [java client] support tables with non-covering partition-key ranges
This commit adds support for accessing and creating tables with non-covering
range partitions to the Java client. The only public interface introduced is the
CreateTableOptions.addRangeBound method. Otherwise, all the changes are
necessary for reading or writing to tables with range partition gaps. Right now
if a client attempts to write to a non-covered range, a NotFound status is
returned, which matches the C++ client. JD pointed out that this makes it
impossible for applications to distinguish between failed updates because the
row doesn't exist, and failed updates because the partition range doesn't exist.
I plan to fix this by introducing a new status code and fixing both clients in a
follow up commit.
Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
---
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
M java/kudu-client/src/main/java/org/kududb/client/Batch.java
M java/kudu-client/src/main/java/org/kududb/client/BatchResponse.java
M java/kudu-client/src/main/java/org/kududb/client/Bytes.java
M java/kudu-client/src/main/java/org/kududb/client/CreateTableOptions.java
M java/kudu-client/src/main/java/org/kududb/client/CreateTableRequest.java
M java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java
M java/kudu-client/src/main/java/org/kududb/client/KuduRpcResponse.java
A java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeCache.java
A java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeException.java
M java/kudu-client/src/main/java/org/kududb/client/Operation.java
M java/kudu-client/src/main/java/org/kududb/client/OperationResponse.java
M java/kudu-client/src/main/java/org/kududb/client/Partition.java
M java/kudu-client/src/main/java/org/kududb/client/RowError.java
M java/kudu-client/src/main/java/org/kududb/client/RowResultIterator.java
M java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/kududb/client/TestFlexiblePartitioning.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduSession.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduTable.java
22 files changed, 830 insertions(+), 138 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/3388/7
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
Patch Set 2:
(35 comments)
http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java:
PS2, Line 139: This map
: * is always the first to be updated, because that's the map from
: * which all the lookups are done in the fast-path of the requests
: * that need to locate a tablet. The second map to be updated is
: * tablet2client, because it comes second in the fast-path
: * of every requests that need to locate a tablet. The third map
: * is only used to handle TabletServer disconnections gracefully.
Nit: reading this again (I reviewed it eons ago), I think this part of the comment should broken up and spread across the different map's comments.
Line 155: * Currently once a non-covered range is added to the cache, it is never
Nit: prefix this part of the comment with "TODO:" since it is expected to change soon.
Line 576: Deferred<AsyncKuduScanner.Response> openScanner(final AsyncKuduScanner scanner) {
Do we need to preserve this indirection due to class/method visibility? If not, perhaps we can remove it?
Line 685: // Otherwise fall through to below where a GetTableLocations lookup will occur
Nit: end with a period.
Line 1148: Pair<byte[], byte[]> nonCoveredRange = getNonCoveredRange(table.getTableId(), key);
Nit: if you want, you can call getTableId() once outside of the loop, store it in a local variable, and use it here and on L1141.
PS2, Line 1355: NonCoveredRangeCache nonCoveredRanges = nonCoveredRangeCaches.get(tableId);
: if (nonCoveredRanges == null) {
: nonCoveredRanges = new NonCoveredRangeCache();
: NonCoveredRangeCache oldCache = nonCoveredRangeCaches.putIfAbsent(tableId, nonCoveredRanges);
: if (oldCache != null) {
: nonCoveredRanges = oldCache;
: }
: }
How about just:
NonCoveredRangeCache nonCoveredRanges = nonCoveredRangeCaches.putIfAbsent(tableId, new NonCoveredRangeCache());
Line 1378: nonCoveredRanges.addNonCoveredRange(EMPTY_ARRAY, firstStartKey);
Don't we need all of these calls to addNonCoveredRange() to be atomic? Without that, isn't it possible for the non-covered range view of a table to become inconsistent?
Imagine two callers in this function. One has an up-to-date locations list, and the other has a stale one (i.e. some add/drop range calls happened in between). All of these addNonCoveredRange() calls get interspersed. The resulting view is totally wacky, no?
http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduScanner.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduScanner.java:
Line 474: invalidate();
Nit: invalidate() is called in both branches, can we pull it out?
http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java:
Line 127: private final List<OperationResponse> failedLookups = new ArrayList<>();
I think you can use a CopyOnWriteArrayList here and omit the use of a lock if you're careful to add all of the failed lookups en masse. That is, add them to an intermediary list, construct a new CopyOnWriteArrayList using the intermediary (because you don't want to add entries one by one to a CopyOnWriteArrayList), then assign it to failedLookups.
On second thought, I'm not sure this is possible because of the atomic "get all and clear" operation in ConvertBatchToListOfResponsesCB. I don't see how to handle that without wrapping the list in an AtomicReference which, while unintuitive, isn't necessarily a bad idea.
Line 279: if (!operationsInLookup.isEmpty()) {
Deferreds are weird, man. I've read the code in the two branches here over and over, and I still can't figure out how the mere semantic change of "call flushAllBatches() immediately" vs. "return a new Deferred with a chained callback to flushAllBatches() via OperationsInLookupDoneCB" is enough to handle the case of outstanding lookups.
OK, I think I figured it out. lookupsDone is a class field and we do other stuff with it elsewhere.
Line 303: Callback<Deferred<List<OperationResponse>>, ArrayList<BatchResponse>> {
Nit: not related to your change, but why is ArrayList the second type and not List?
Ahh, it's because Deferred.group() in flushAllBatches() returns a Deferred<ArrayList>. Wonderful.
Line 306: batchResponses = batchResponses == null ? new ArrayList<BatchResponse>() : batchResponses;
Is a null batchResponses actually possible? How?
PS2, Line 308: // flushTablet() can return null when a tablet we wanted to flush was already flushed. Those
: // nulls along with BatchResponses are then put in a list by Deferred.group(). We first need
: // to filter the nulls out.
OMG.
Line 320: List<OperationResponse> responsesList = new ArrayList<>(size);
If we're going to go through the trouble of precomputing the size, we ought to include failedLookups too.
Line 397: if (tablet == null) {
Why don't we perform this check to AUTO_FLUSH_SYNC on L377-384? Seems useful to figure out locally that we're going to write to a non-covered range than to make the round trip to the TS to learn the same thing.
PS2, Line 399: new RowError(Status.NotFound("Write to a non-covered range partition"),
: operation, null);
Could you add a two-arg constructor for this case?
PS2, Line 401: new OperationResponse(0, null, 0, operation, rowError);
Likewise, perhaps add a new constructor for this?
PS2, Line 416: if (lookupsDone != null && operationsInLookup.isEmpty()) {
: lookupsDoneCopy = lookupsDone;
: lookupsDone = null;
: }
Copied from addToBuffer(), right? Can it be decomposed into a helper with an intuitive name? Ideally it'd do the remove() too, but addToBuffer() wants to know if remove() returned anything, so then the helper would need to return a tuple (or a special pair class, or whatever).
Line 422: operation.callback(response);
Why do we do this?
http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/main/java/org/kududb/client/CreateTableOptions.java
File java/kudu-client/src/main/java/org/kududb/client/CreateTableOptions.java:
Line 57: * The table creator takes ownership of the rows. If either row is empty, then
Is this copied from the C++ client? What does 'ownership' mean here? In any event, why not make copies of the rows like addSplitRow()?
http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java
File java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java:
Line 77: return null;
Why return null instead of throwing some sort of "unsupported operation" exception, here in the base class?
http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeCache.java
File java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeCache.java:
Line 39: final class NonCoveredRangeCache {
Nit: why 'final'? The class is package-private, so it seems like the only classes whom we're preventing from overriding are classes that are part of the client itself.
Line 40: public static final Logger LOG = LoggerFactory.getLogger(NonCoveredRangeCache.class);
Nit: can be private
Line 59: if (COMPARATOR.compare(partitionKey, range.getKey()) >= 0 &&
Isn't this first comparison implied by the floorEntry() call?
Line 61: return new Pair<>(range.getKey(), range.getValue());
Why can't we return the range as-is? Why do we need to make a copy?
Line 77: if (nonCoveredRanges.put(startPartitionKey, endPartitionKey) == null) {
May want to comment on the concurrent behavior (i.e. multiple callers in discoverNonCoveredRanges() at the same time). Seems like there'll be unnecessary put() calls here, but assuming the tablet listing is the same in each caller, in the end the map should have the right contents.
Line 84: public String toString() {
Looks like this will return an inconsistent string representation if nonCoveredRanges changes mid-call (due to the separation of entrySet() and size()). Can you fix that?
Line 89: if (i++ < nonCoveredRanges.size()) sb.append(", ");
If you used a Guava Joiner to convert this list of Strings into a single comma-separated String, you could omit the call to size() altogether.
http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeException.java
File java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeException.java:
Line 26: @InterfaceStability.Unstable
Why not evolving?
http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/main/java/org/kududb/client/RowResultIterator.java
File java/kudu-client/src/main/java/org/kududb/client/RowResultIterator.java:
Line 35: private static final RowResultIterator EMPTY = new RowResultIterator(0, null, null, null, null);
If this is for tests only, perhaps make it more visible, annotate it with @VisibleForTesting, and omit the method exposing it?
http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java
File java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java:
PS2, Line 243: * * [ 0, 50)
: * * [ 50, 100)
: * * [200, 300)
Nit: what does the extra '*' mean?
http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/test/java/org/kududb/client/TestFlexiblePartitioning.java
File java/kudu-client/src/test/java/org/kududb/client/TestFlexiblePartitioning.java:
Line 104: KuduScanner s = syncClient.newScannerBuilder(table).scanRequestTimeout(9999999).build();
That's an odd choice of timeout. Why? And if you're looking for a large number, why not INT_MAX or something equivalent?
http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/test/java/org/kududb/client/TestKuduClient.java
File java/kudu-client/src/test/java/org/kududb/client/TestKuduClient.java:
Line 457: if (key == 0) session.flush();
Why flush after the very first operation? What's the significance of that?
http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/test/java/org/kududb/client/TestKuduSession.java
File java/kudu-client/src/test/java/org/kududb/client/TestKuduSession.java:
Line 35: tableName = TestKuduClient.class.getName() + "-" + System.currentTimeMillis();
FWIW, even though this is guaranteed to run with every test, if a test finishes in under one ms, the next test could end up with the same table name.
Maybe use https://github.com/junit-team/junit4/wiki/Rules#testname-rule to help?
Line 221: createOptions.setNumReplicas(1);
Why do the new tests use only one replica, but the old ones don't?
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java:
Line 576: Deferred<AsyncKuduScanner.Response> openScanner(final AsyncKuduScanner scanner) {
> Do we need to preserve this indirection due to class/method visibility? If
True, this can be removed.
http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java:
Line 279: if (!operationsInLookup.isEmpty()) {
> Deferreds are weird, man. I've read the code in the two branches here over
Yeah it's a tricky one. A lot of what's happening is implied.
http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/main/java/org/kududb/client/RowResultIterator.java
File java/kudu-client/src/main/java/org/kududb/client/RowResultIterator.java:
Line 35: private static final RowResultIterator EMPTY = new RowResultIterator(0, null, null, null, null);
> If this is for tests only, perhaps make it more visible, annotate it with @
It's not, we return it if your end key on the scan is in non-covered range. We could document this.
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
Patch Set 2: -Verified
Build Started http://104.196.14.100/job/kudu-gerrit/1856/
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
Patch Set 8: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
Patch Set 6:
Build Started http://104.196.14.100/job/kudu-gerrit/2122/
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/3388/4/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java:
Line 327: operation.callback(response);
> It will do it right here, since it's the operation's callback were invoking
Dan, I think you missed this discussion. Sounds like there's a missing check on AUTO_FLUSH_BACKGROUND, and perhaps there should be a comment explaining the lack of importance in order between invoking the user's callback and updating the error collector.
http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeCache.java
File java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeCache.java:
Line 84: @Override
> Really? I thought ConcurrentSkipListMap defines an iteration order in the f
I think you missed this.
Line 89: for (Map.Entry<byte[], byte[]> range : nonCoveredRanges.entrySet()) {
> No love for Joiner? Bear in mind size() is more expensive on a ConcurrentSk
Think you missed this.
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
Patch Set 3: -Verified
Build Started http://104.196.14.100/job/kudu-gerrit/1977/
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
Patch Set 3: -Verified
Build Started http://104.196.14.100/job/kudu-gerrit/1978/
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
Patch Set 7:
Build Started http://104.196.14.100/job/kudu-gerrit/2181/
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
Patch Set 4:
Build Started http://104.196.14.100/job/kudu-gerrit/1983/
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3388
to look at the new patch set (#6).
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
KUDU-1309: [java client] support tables with non-covering partition-key ranges
This commit adds support for accessing and creating tables with non-covering
range partitions to the Java client. The only public interface introduced is the
CreateTableOptions.addRangeBound method. Otherwise, all the changes are
necessary for reading or writing to tables with range partition gaps. Right now
if a client attempts to write to a non-covered range, a NotFound status is
returned, which matches the C++ client. JD pointed out that this makes it
impossible for applications to distinguish between failed updates because the
row doesn't exist, and failed updates because the partition range doesn't exist.
I plan to fix this by introducing a new status code and fixing both clients in a
follow up commit.
Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
---
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
M java/kudu-client/src/main/java/org/kududb/client/Batch.java
M java/kudu-client/src/main/java/org/kududb/client/BatchResponse.java
M java/kudu-client/src/main/java/org/kududb/client/Bytes.java
M java/kudu-client/src/main/java/org/kududb/client/CreateTableOptions.java
M java/kudu-client/src/main/java/org/kududb/client/CreateTableRequest.java
M java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java
M java/kudu-client/src/main/java/org/kududb/client/KuduRpcResponse.java
A java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeCache.java
A java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeException.java
M java/kudu-client/src/main/java/org/kududb/client/Operation.java
M java/kudu-client/src/main/java/org/kududb/client/OperationResponse.java
M java/kudu-client/src/main/java/org/kududb/client/Partition.java
M java/kudu-client/src/main/java/org/kududb/client/RowError.java
M java/kudu-client/src/main/java/org/kududb/client/RowResultIterator.java
M java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/kududb/client/TestFlexiblePartitioning.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduSession.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduTable.java
22 files changed, 836 insertions(+), 145 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/3388/6
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
Patch Set 1: -Verified
Build Started http://104.196.14.100/job/kudu-gerrit/1832/
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has submitted this change and it was merged.
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
KUDU-1309: [java client] support tables with non-covering partition-key ranges
This commit adds support for accessing and creating tables with non-covering
range partitions to the Java client. The only public interface introduced is the
CreateTableOptions.addRangeBound method. Otherwise, all the changes are
necessary for reading or writing to tables with range partition gaps. Right now
if a client attempts to write to a non-covered range, a NotFound status is
returned, which matches the C++ client. JD pointed out that this makes it
impossible for applications to distinguish between failed updates because the
row doesn't exist, and failed updates because the partition range doesn't exist.
I plan to fix this by introducing a new status code and fixing both clients in a
follow up commit.
Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Reviewed-on: http://gerrit.cloudera.org:8080/3388
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Jean-Daniel Cryans <jd...@apache.org>
---
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
M java/kudu-client/src/main/java/org/kududb/client/Batch.java
M java/kudu-client/src/main/java/org/kududb/client/BatchResponse.java
M java/kudu-client/src/main/java/org/kududb/client/Bytes.java
M java/kudu-client/src/main/java/org/kududb/client/CreateTableOptions.java
M java/kudu-client/src/main/java/org/kududb/client/CreateTableRequest.java
M java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java
M java/kudu-client/src/main/java/org/kududb/client/KuduRpcResponse.java
A java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeCache.java
A java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeException.java
M java/kudu-client/src/main/java/org/kududb/client/Operation.java
M java/kudu-client/src/main/java/org/kududb/client/OperationResponse.java
M java/kudu-client/src/main/java/org/kududb/client/Partition.java
M java/kudu-client/src/main/java/org/kududb/client/RowError.java
M java/kudu-client/src/main/java/org/kududb/client/RowResultIterator.java
M java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/kududb/client/TestFlexiblePartitioning.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduSession.java
M java/kudu-client/src/test/java/org/kududb/client/TestKuduTable.java
22 files changed, 839 insertions(+), 138 deletions(-)
Approvals:
Jean-Daniel Cryans: Looks good to me, approved
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1309: [java client] support tables with non-covering partition-key ranges
......................................................................
Patch Set 6:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/3388/4/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java:
Line 810: } else {
> This will need a rebase + conflict resolution.
Done
Line 1152: Map.Entry<byte[], byte[]> nonCoveredRange = getNonCoveredRange(tableId, key);
> Can use tableId here.
Done
http://gerrit.cloudera.org:8080/#/c/3388/4/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java:
PS4, Line 784: private Object tablet = null;
> Ugh. Can you add more color on why this approach was necessary? Why can't w
It could be done that way, but the downcasting would need to happen in the callback, so it's really just moving the ugliness. I personally think having it wrapped up in this small class is good enough containment of the obviously poor abstraction.
--
To view, visit http://gerrit.cloudera.org:8080/3388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes