[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
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 Reviewed-by: Jean-Daniel Cryans --- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert 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
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert 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
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 range :
nonCoveredRanges.entrySet()) {
> Think you missed this.
yah I'm not crazy about Joiner unless there is already a List.
--
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
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
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert 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
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
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 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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
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
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert 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
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
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 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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
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
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert 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
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert 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
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert 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
Adar Dembo has posted comments on this change.
Change subject: KUDU-1309: [java client] support tables with non-covering
partition-key ranges
..
Patch Set 4:
(7 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.
Line 1152: Map.Entry nonCoveredRange =
getNonCoveredRange(table.getTableId(), key);
Can use tableId here.
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 adding
the error to the error collector?
PS4, Line 784: private Object tablet = null;
Ugh. Can you add more color on why this approach was necessary? Why can't we
store the results in two separate fields, one LocatedTablet and one exception?
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
> if concurrent updates happen to the entries the iteration is undefined, so
Really? I thought ConcurrentSkipListMap defines an iteration order in the face
of concurrent mutations. Javadoc says: "Insertion, removal, update, and access
operations safely execute concurrently by multiple threads. Iterators are
weakly consistent, returning elements reflecting the state of the map at some
point at or since the creation of the iterator. They do not throw
ConcurrentModificationException, and may proceed concurrently with other
operations."
Line 89: for (Map.Entry range :
nonCoveredRanges.entrySet()) {
> If you used a Guava Joiner to convert this list of Strings into a single co
No love for Joiner? Bear in mind size() is more expensive on a
ConcurrentSkipListMap.
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 221: public void testInsertManualFlushNonCoveredRange() throws Exception
{
> Replicas aren't important for these particular tests. Additionally, they c
Right, but I don't think the other tests in this class particularly care about
replication either (testUpsert() for example). Could you make a pass over all
of the TestKuduSession tests and switch every test to single-replica, if it
makes sense? It's more cognitive load as it is now; the reader's eyes are drawn
to the inconsistency.
--
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
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
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert 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
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert 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
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert 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
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 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 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 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 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>,
ArrayList> {
> Nit: not related to your change, but why is ArrayList the second type and n
Done
Line 306: batchResponses = batchResponses == null ? new
ArrayList() : 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 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, d
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert 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
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert 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
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 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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
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
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 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 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 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>,
ArrayList> {
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. Wonderful.
Line 306: batchResponses = batchResponses == null ? new
ArrayList() : batchResponses;
Is a null batchResponses actually possible? How?
PS2, Line 308: //
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert 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
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
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
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
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
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 nega
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
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 Gerrit-Reviewer: Adar Dembo 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
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans
[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
