You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Grant Henke (Code Review)" <ge...@cloudera.org> on 2019/03/08 21:07:05 UTC
[kudu-CR] [java] Allow delete operations with extra columns set
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12705
Change subject: [java] Allow delete operations with extra columns set
......................................................................
[java] Allow delete operations with extra columns set
Before this patch, a delete operation required that the
key columns and only the key columns are set. If any
additional column was set, the operation would fail
server side.
With this patch, any non-key columns are stripped
when encoding the row. This allows extra columns to
be set in a delete operation client side without inflating the
message on the wire.
Change-Id: Id302f349a35b9faf7382c74f9a3823f8e76f000c
---
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduSession.java
2 files changed, 47 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/12705/1
--
To view, visit http://gerrit.cloudera.org:8080/12705
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id302f349a35b9faf7382c74f9a3823f8e76f000c
Gerrit-Change-Number: 12705
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
[kudu-CR] [java] Allow delete operations with extra columns set
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12705 )
Change subject: [java] Allow delete operations with extra columns set
......................................................................
[java] Allow delete operations with extra columns set
Before this patch, a delete operation required that the
key columns and only the key columns are set. If any
additional column was set, the operation would fail
server side.
With this patch, any non-key columns are stripped
when encoding the row. This allows extra columns to
be set in a delete operation client side without inflating the
message on the wire.
Change-Id: Id302f349a35b9faf7382c74f9a3823f8e76f000c
Reviewed-on: http://gerrit.cloudera.org:8080/12705
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduSession.java
2 files changed, 47 insertions(+), 7 deletions(-)
Approvals:
Kudu Jenkins: Verified
Alexey Serbin: Looks good to me, but someone else must approve
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/12705
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Id302f349a35b9faf7382c74f9a3823f8e76f000c
Gerrit-Change-Number: 12705
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [java] Allow delete operations with extra columns set
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12705 )
Change subject: [java] Allow delete operations with extra columns set
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12705/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12705/1//COMMIT_MSG@14
PS1, Line 14: With this patch, any non-key columns are stripped
: when encoding the row.
I'm curious why not to enforce the same requirements for the row at the client side as at the server, i.e. report an error instead of silently stripping all non-key columns. I understand that adds more restrictions, but it would be more consistent with the rest of the verification steps performed at the client side.
Is there any motivation for this alternative approach that comes from real-world scenarios?
--
To view, visit http://gerrit.cloudera.org:8080/12705
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id302f349a35b9faf7382c74f9a3823f8e76f000c
Gerrit-Change-Number: 12705
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 09 Mar 2019 00:38:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Allow delete operations with extra columns set
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12705 )
Change subject: [java] Allow delete operations with extra columns set
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12705/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12705/1//COMMIT_MSG@14
PS1, Line 14: With this patch, any non-key columns are stripped
: when encoding the row.
> I'm curious why not to enforce the same requirements for the row at the cli
+1, I had also asked Grant the same question. I think he'd rather the client be permissive than restrictive.
--
To view, visit http://gerrit.cloudera.org:8080/12705
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id302f349a35b9faf7382c74f9a3823f8e76f000c
Gerrit-Change-Number: 12705
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 09 Mar 2019 00:48:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Allow delete operations with extra columns set
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12705 )
Change subject: [java] Allow delete operations with extra columns set
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12705/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12705/1//COMMIT_MSG@14
PS1, Line 14: With this patch, any non-key columns are stripped
: when encoding the row.
> I think the client-side handling boils down to personal taste.
I'd vote for the stricter behavior at the client side as well, but that requires much more effort, as I understand. Maybe, it makes sense to add a JIRA to implement that as a future improvement?
+1 add for extending this behavior for the C++ client as well.
--
To view, visit http://gerrit.cloudera.org:8080/12705
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id302f349a35b9faf7382c74f9a3823f8e76f000c
Gerrit-Change-Number: 12705
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 11 Mar 2019 21:50:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Allow delete operations with extra columns set
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12705 )
Change subject: [java] Allow delete operations with extra columns set
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12705/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12705/1//COMMIT_MSG@14
PS1, Line 14: With this patch, any non-key columns are stripped
: when encoding the row.
> Honestly my primary motivation is simplified testing. That said, I often fi
I think it makes sense to keep the server-side verification strict to at least avoid accepting junk data from the client-side. Also, keeping server-side strict allows to spot unintended misuse of the API.
For the client-side I think it's OK (or maybe even better?) to be more accommodating for use-cases coming from the real world. It's great that you can envision some use-cases that would benefit from this change.
Maybe, Adar has some additional thoughts on what's good for the client side.
--
To view, visit http://gerrit.cloudera.org:8080/12705
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id302f349a35b9faf7382c74f9a3823f8e76f000c
Gerrit-Change-Number: 12705
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 11 Mar 2019 17:35:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Allow delete operations with extra columns set
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12705 )
Change subject: [java] Allow delete operations with extra columns set
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12705/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12705/1//COMMIT_MSG@14
PS1, Line 14: With this patch, any non-key columns are stripped
: when encoding the row.
> I think it makes sense to keep the server-side verification strict to at le
I think the client-side handling boils down to personal taste.
I myself would have opted for stricter defaults with some additional tooling to support such cases. Meaning, the client would reject a DELETE with non-primary key data, but PartialRow offers the API Grant mentioned to drop non-primary key data. Taken together, we'd be able to support the use cases Grant mentioned while still preserving consistent behavior between the Java client and the server (as well as the Java client and the C++ client, which continues to send the raw data to the server and relies on its error handling).
But this is fine too. I imagine Grant will extend this to the C++ client next, to keep its behavior consistent with the Java client.
--
To view, visit http://gerrit.cloudera.org:8080/12705
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id302f349a35b9faf7382c74f9a3823f8e76f000c
Gerrit-Change-Number: 12705
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 11 Mar 2019 18:04:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Allow delete operations with extra columns set
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12705 )
Change subject: [java] Allow delete operations with extra columns set
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12705/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12705/1//COMMIT_MSG@14
PS1, Line 14: With this patch, any non-key columns are stripped
: when encoding the row.
Honestly my primary motivation is simplified testing. That said, I often find things that are useful for testing can be useful in real world use cases too.
I think supporting permissive deletes could simplify replication/cdc type use cases where data is read from one data store (maybe Kudu) and then written to Kudu. If we are strict, the user needs to project just the key columns in this process. We also don't currently have a good API to take an existing PartitalRow and drop all but the key columns.
> I'm curious why not to enforce the same requirements for the row at the client side as at the server
I agree that the server can/should be more permissive too. Even with that the client side change reduces the message size on the wire.
--
To view, visit http://gerrit.cloudera.org:8080/12705
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id302f349a35b9faf7382c74f9a3823f8e76f000c
Gerrit-Change-Number: 12705
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 11 Mar 2019 15:58:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Allow delete operations with extra columns set
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12705 )
Change subject: [java] Allow delete operations with extra columns set
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12705
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id302f349a35b9faf7382c74f9a3823f8e76f000c
Gerrit-Change-Number: 12705
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 11 Mar 2019 18:04:49 +0000
Gerrit-HasComments: No