You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org> on 2023/06/12 21:16:24 UTC
[kudu-CR] [server] KUDU-1945 Auto incrementing column Upsert support
Abhishek Chennaka has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20061
Change subject: [server] KUDU-1945 Auto_incrementing column Upsert support
......................................................................
[server] KUDU-1945 Auto_incrementing column Upsert support
Add Upsert operation support on server side when the entire row is present
in the request including the auto-incrementing column.
There will be followup patches to update the clients and test the
corresponding changes as well.
Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
---
M src/kudu/common/row_operations.cc
M src/kudu/tablet/tablet_auto_incrementing-test.cc
2 files changed, 109 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/20061/1
--
To view, visit http://gerrit.cloudera.org:8080/20061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
Gerrit-Change-Number: 20061
Gerrit-PatchSet: 1
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
[kudu-CR] [server] KUDU-1945 Auto incrementing column UPSERT support
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: [server] KUDU-1945 Auto_incrementing column UPSERT support
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/20061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
Gerrit-Change-Number: 20061
Gerrit-PatchSet: 10
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [server] KUDU-1945 Auto incrementing column UPSERT support
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/20061 )
Change subject: [server] KUDU-1945 Auto_incrementing column UPSERT support
......................................................................
Patch Set 10:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/20061/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20061/6//COMMIT_MSG@14
PS6, Line 14: With this patch the expected behavior for different operations
: from Kudu clients is as follows:
> This is hard to parse, please update this to be more readable.
Done
http://gerrit.cloudera.org:8080/#/c/20061/6//COMMIT_MSG@23
PS6, Line 23: implementati
> implementation
Done
http://gerrit.cloudera.org:8080/#/c/20061/8/src/kudu/tablet/tablet_auto_incrementing-test.cc
File src/kudu/tablet/tablet_auto_incrementing-test.cc:
http://gerrit.cloudera.org:8080/#/c/20061/8/src/kudu/tablet/tablet_auto_incrementing-test.cc@19
PS8, Line 19: #include <memory>
> nit: remove empty line 19
Done
http://gerrit.cloudera.org:8080/#/c/20061/8/src/kudu/tablet/tablet_auto_incrementing-test.cc@189
PS8, Line 189: unique_ptr<KuduPartialRow> row(new KuduPartialRow(&client_schema_));
: ASSERT_OK(row->SetInt64(0, INT64_MAX));
: ASSERT_OK(row->SetInt32(1, 1337));
: ASSERT_OK(writer_->Upsert(*row));
: }
: {
> Thank you for adding these negative scenarios!
Done
--
To view, visit http://gerrit.cloudera.org:8080/20061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
Gerrit-Change-Number: 20061
Gerrit-PatchSet: 10
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Fri, 23 Jun 2023 17:41:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] [server] KUDU-1945 Auto incrementing column UPSERT support
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Marton Greber, Alexey Serbin, Yuqi Du, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20061
to look at the new patch set (#5).
Change subject: [server] KUDU-1945 Auto_incrementing column UPSERT support
......................................................................
[server] KUDU-1945 Auto_incrementing column UPSERT support
Add UPSERT operation support on the server side when the entire row
is present in the request including the auto-incrementing column.
There will be followup patches to update the clients and test the
corresponding changes as well.
After this patch below is the different operation support for
tables with auto-incrementing columns from the Kudu client.
Operation Presence of auto-incrmeneting column
INSERT/INSERT_IGNORE Should not be specified
UPDATE/UPDATE_IGNORE Has to be specified
UPSERT/UPSERT_IGNORE Has to be specified
DELETE/DELETE_IGNORE Has to be specified
This implemention of UPSERT operation behavior in this patch
is to facilitate incremental restores consistently when using
Kudu backup and restore tool.
More details in the design document:
https://docs.google.com/document/d/1-x6F5TkRYMqt2umDBQ5x-Aa4dqKwFYpbxo1A6QoMafo/edit#heading=h.vnagsa4oxwrf
Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
---
M python/kudu/tests/test_client.py
M src/kudu/common/row_operations.cc
M src/kudu/tablet/tablet_auto_incrementing-test.cc
3 files changed, 170 insertions(+), 50 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/20061/5
--
To view, visit http://gerrit.cloudera.org:8080/20061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
Gerrit-Change-Number: 20061
Gerrit-PatchSet: 5
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [server] KUDU-1945 Auto incrementing column UPSERT support
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/20061 )
Change subject: [server] KUDU-1945 Auto_incrementing column UPSERT support
......................................................................
[server] KUDU-1945 Auto_incrementing column UPSERT support
Add UPSERT operation support on the server side when the entire row
is present in the request including the auto-incrementing column.
There will be followup patches to update the clients and test the
corresponding changes as well.
With this patch the expected behavior for different operations
from Kudu clients is as follows:
Operation Presence of auto-incrementing column
INSERT/INSERT_IGNORE Should not be specified
UPDATE/UPDATE_IGNORE Has to be specified
UPSERT/UPSERT_IGNORE Has to be specified
DELETE/DELETE_IGNORE Has to be specified
This implementation of UPSERT operation behavior in this patch
is to facilitate incremental restores consistently when using
Kudu backup and restore tool.
More details in the design document:
https://docs.google.com/document/d/1-x6F5TkRYMqt2umDBQ5x-Aa4dqKwFYpbxo1A6QoMafo/edit#heading=h.vnagsa4oxwrf
Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
Reviewed-on: http://gerrit.cloudera.org:8080/20061
Reviewed-by: Alexey Serbin <al...@apache.org>
Tested-by: Alexey Serbin <al...@apache.org>
---
M python/kudu/tests/test_client.py
M src/kudu/common/row_operations.cc
M src/kudu/tablet/tablet_auto_incrementing-test.cc
3 files changed, 203 insertions(+), 50 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/20061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
Gerrit-Change-Number: 20061
Gerrit-PatchSet: 11
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [server] KUDU-1945 Auto incrementing column UPSERT support
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Marton Greber, Alexey Serbin, Yuqi Du, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20061
to look at the new patch set (#7).
Change subject: [server] KUDU-1945 Auto_incrementing column UPSERT support
......................................................................
[server] KUDU-1945 Auto_incrementing column UPSERT support
Add UPSERT operation support on the server side when the entire row
is present in the request including the auto-incrementing column.
There will be followup patches to update the clients and test the
corresponding changes as well.
After this patch below is the different operation support for
tables with auto-incrementing columns from the Kudu client.
Operation Presence of auto-incrmeneting column
INSERT/INSERT_IGNORE Should not be specified
UPDATE/UPDATE_IGNORE Has to be specified
UPSERT/UPSERT_IGNORE Has to be specified
DELETE/DELETE_IGNORE Has to be specified
This implemention of UPSERT operation behavior in this patch
is to facilitate incremental restores consistently when using
Kudu backup and restore tool.
More details in the design document:
https://docs.google.com/document/d/1-x6F5TkRYMqt2umDBQ5x-Aa4dqKwFYpbxo1A6QoMafo/edit#heading=h.vnagsa4oxwrf
Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
---
M python/kudu/tests/test_client.py
M src/kudu/common/row_operations.cc
M src/kudu/tablet/tablet_auto_incrementing-test.cc
3 files changed, 172 insertions(+), 50 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/20061/7
--
To view, visit http://gerrit.cloudera.org:8080/20061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
Gerrit-Change-Number: 20061
Gerrit-PatchSet: 7
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [server] KUDU-1945 Auto incrementing column Upsert support
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/20061 )
Change subject: [server] KUDU-1945 Auto_incrementing column Upsert support
......................................................................
Patch Set 3:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/20061/2//COMMIT_MSG
Commit Message:
PS2:
> It would be great to outline the matrix of the operations where specifying
Done
http://gerrit.cloudera.org:8080/#/c/20061/2//COMMIT_MSG@7
PS2, Line 7: Upsert
> UPSERT
Done
http://gerrit.cloudera.org:8080/#/c/20061/2//COMMIT_MSG@9
PS2, Line 9: server side
> the server side
Done
http://gerrit.cloudera.org:8080/#/c/20061/2//COMMIT_MSG@9
PS2, Line 9: Upsert
> UPSERT
Done
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc
File src/kudu/common/row_operations.cc:
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@477
PS2, Line 477: PREDICT_FALSE
> It's not a good idea to use PREDICT_FALSE for conditions that are attribute
Done
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@484
PS2, Line 484: uint8_t *
> correct this to be style-compliant (maybe, it's time to configure your IDE/
Done
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@490
PS2, Line 490: err_value
> https://google.github.io/styleguide/cppguide.html#Constant_Names
Done, thanks
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@490
PS2, Line 490: IllegalState
> Should this be InvalidArgument if it's coming from the write request?
Done
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@490
PS2, Line 490: err_value
> Correct this to be style-compliant (should be in form cErrValue or somethin
Done
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@502
PS2, Line 502: }
> Make this first element of the corresponding 'if ()' clause, and since ther
Done
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@504
PS2, Line 504: column values not to be
> Correct this to be style-compliant.
Done
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@511
PS2, Line 511: } else {
: // Copy the non-null value.
: Status row_status;
: RETURN_NOT_OK(ReadColumn(
: col, tablet_row.mutable_cell_ptr(tablet_col_idx), &row_status));
: if (PREDICT_FALSE(!row_status.ok())) {
:
> Make this first part of the corresponding if/else for better readability si
Done
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@796
PS2, Line 796: case RowOperationsPB::UPSERT_IGNORE:
: /* if (tablet_schema_->has_auto_incrementing()) {
: return Status::NotSupported(
: Substitute("tables with auto-incrementing column do not support "
:
> If this code isn't needed, just remove it, don't comment out (especially wi
Ah, this was meant to be removed. Thanks for catching it.
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/tablet/tablet_auto_incrementing-test.cc
File src/kudu/tablet/tablet_auto_incrementing-test.cc:
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/tablet/tablet_auto_incrementing-test.cc@161
PS2, Line 161: }
> It would be great to add some coverage for the following conditions:
Done
--
To view, visit http://gerrit.cloudera.org:8080/20061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
Gerrit-Change-Number: 20061
Gerrit-PatchSet: 3
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Wed, 21 Jun 2023 21:39:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] [server] KUDU-1945 Auto incrementing column Upsert support
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Marton Greber, Alexey Serbin, Yuqi Du, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20061
to look at the new patch set (#2).
Change subject: [server] KUDU-1945 Auto_incrementing column Upsert support
......................................................................
[server] KUDU-1945 Auto_incrementing column Upsert support
Add Upsert operation support on server side when the entire row is present
in the request including the auto-incrementing column.
There will be followup patches to update the clients and test the
corresponding changes as well.
Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
---
M src/kudu/common/row_operations.cc
M src/kudu/tablet/tablet_auto_incrementing-test.cc
2 files changed, 109 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/20061/2
--
To view, visit http://gerrit.cloudera.org:8080/20061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
Gerrit-Change-Number: 20061
Gerrit-PatchSet: 2
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [server] KUDU-1945 Auto incrementing column Upsert support
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Marton Greber, Alexey Serbin, Yuqi Du, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20061
to look at the new patch set (#3).
Change subject: [server] KUDU-1945 Auto_incrementing column Upsert support
......................................................................
[server] KUDU-1945 Auto_incrementing column Upsert support
Add Upsert operation support on server side when the entire row is present
in the request including the auto-incrementing column.
There will be followup patches to update the clients and test the
corresponding changes as well.
Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
---
M src/kudu/common/row_operations.cc
M src/kudu/tablet/tablet_auto_incrementing-test.cc
2 files changed, 110 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/20061/3
--
To view, visit http://gerrit.cloudera.org:8080/20061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
Gerrit-Change-Number: 20061
Gerrit-PatchSet: 3
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [server] KUDU-1945 Auto incrementing column Upsert support
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/20061 )
Change subject: [server] KUDU-1945 Auto_incrementing column Upsert support
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc
File src/kudu/common/row_operations.cc:
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@490
PS2, Line 490: "invalid auto-incrementing "
: "column value"
> Since we are in the if block, maybe we could give more detailed error messa
Done
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@505
PS2, Line 505: "auto-incrementing column is incorrectly set"
> Same thing as above, maybe we can give more detailed error message, for exa
Done
--
To view, visit http://gerrit.cloudera.org:8080/20061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
Gerrit-Change-Number: 20061
Gerrit-PatchSet: 2
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Fri, 16 Jun 2023 18:04:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] [server] KUDU-1945 Auto incrementing column UPSERT support
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Marton Greber, Alexey Serbin, Yuqi Du, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20061
to look at the new patch set (#9).
Change subject: [server] KUDU-1945 Auto_incrementing column UPSERT support
......................................................................
[server] KUDU-1945 Auto_incrementing column UPSERT support
Add UPSERT operation support on the server side when the entire row
is present in the request including the auto-incrementing column.
There will be followup patches to update the clients and test the
corresponding changes as well.
With this patch the expected behavior for different operations
from Kudu clients is as follows:
Operation Presence of auto-incrementing column
INSERT/INSERT_IGNORE Should not be specified
UPDATE/UPDATE_IGNORE Has to be specified
UPSERT/UPSERT_IGNORE Has to be specified
DELETE/DELETE_IGNORE Has to be specified
This implemention of UPSERT operation behavior in this patch
is to facilitate incremental restores consistently when using
Kudu backup and restore tool.
More details in the design document:
https://docs.google.com/document/d/1-x6F5TkRYMqt2umDBQ5x-Aa4dqKwFYpbxo1A6QoMafo/edit#heading=h.vnagsa4oxwrf
Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
---
M python/kudu/tests/test_client.py
M src/kudu/common/row_operations.cc
M src/kudu/tablet/tablet_auto_incrementing-test.cc
3 files changed, 203 insertions(+), 50 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/20061/9
--
To view, visit http://gerrit.cloudera.org:8080/20061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
Gerrit-Change-Number: 20061
Gerrit-PatchSet: 9
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [server] KUDU-1945 Auto incrementing column UPSERT support
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Marton Greber, Alexey Serbin, Yuqi Du, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20061
to look at the new patch set (#4).
Change subject: [server] KUDU-1945 Auto_incrementing column UPSERT support
......................................................................
[server] KUDU-1945 Auto_incrementing column UPSERT support
Add UPSERT operation support on the server side when the entire row
is present in the request including the auto-incrementing column.
There will be followup patches to update the clients and test the
corresponding changes as well.
After this patch below is the different operation support for
tables with auto-incrementing columns from the Kudu client.
Operation Presence of auto-incrmeneting column
INSERT/INSERT_IGNORE Should not be specified
UPDATE/UPDATE_IGNORE Has to be specified
UPSERT/UPSERT_IGNORE Has to be specified
DELETE/DELETE_IGNORE Has to be specified
This implemention of UPSERT operation behavior in this patch
is to facilitate incremental restores consistently when using
Kudu backup and restore tool.
More details in the design document:
https://docs.google.com/document/d/1-x6F5TkRYMqt2umDBQ5x-Aa4dqKwFYpbxo1A6QoMafo/edit#heading=h.vnagsa4oxwrf
Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
---
M src/kudu/common/row_operations.cc
M src/kudu/tablet/tablet_auto_incrementing-test.cc
2 files changed, 169 insertions(+), 49 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/20061/4
--
To view, visit http://gerrit.cloudera.org:8080/20061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
Gerrit-Change-Number: 20061
Gerrit-PatchSet: 4
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [server] KUDU-1945 Auto incrementing column Upsert support
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/20061 )
Change subject: [server] KUDU-1945 Auto_incrementing column Upsert support
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc
File src/kudu/common/row_operations.cc:
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@490
PS2, Line 490: "invalid auto-incrementing "
: "column value"
Since we are in the if block, maybe we could give more detailed error message, for example something like: "auto-incrementing column value must be greater than zero"?
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@505
PS2, Line 505: "auto-incrementing column is incorrectly set"
Same thing as above, maybe we can give more detailed error message, for example something like "can't set auto-incrementing column value unless it is upsert or upsert ignore operation"?
--
To view, visit http://gerrit.cloudera.org:8080/20061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
Gerrit-Change-Number: 20061
Gerrit-PatchSet: 2
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Fri, 16 Jun 2023 14:29:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] [server] KUDU-1945 Auto incrementing column Upsert support
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20061 )
Change subject: [server] KUDU-1945 Auto_incrementing column Upsert support
......................................................................
Patch Set 2:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/20061/2//COMMIT_MSG
Commit Message:
PS2:
It would be great to outline the matrix of the operations where specifying the value for an auto-incrementing column is allowed/disallowed.
Also, providing a reasoning for the decision to allow/disallow specifying a value for the auto-incrementing column would be great, or provide a link to the design document instead where the such reasoning is present.
http://gerrit.cloudera.org:8080/#/c/20061/2//COMMIT_MSG@7
PS2, Line 7: Upsert
UPSERT
http://gerrit.cloudera.org:8080/#/c/20061/2//COMMIT_MSG@9
PS2, Line 9: Upsert
UPSERT
http://gerrit.cloudera.org:8080/#/c/20061/2//COMMIT_MSG@9
PS2, Line 9: server side
the server side
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc
File src/kudu/common/row_operations.cc:
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@477
PS2, Line 477: PREDICT_FALSE
It's not a good idea to use PREDICT_FALSE for conditions that are attributed to regular and designed (i.e. not exceptional) workflow.
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@484
PS2, Line 484: uint8_t *
correct this to be style-compliant (maybe, it's time to configure your IDE/editor?)
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@490
PS2, Line 490: err_value
Correct this to be style-compliant (should be in form cErrValue or something).
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@490
PS2, Line 490: IllegalState
Should this be InvalidArgument if it's coming from the write request?
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@502
PS2, Line 502: } else {
Make this first element of the corresponding 'if ()' clause, and since there is 'return' from this scope, it will be more readable then when removing the if/else scope for then the second part.
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@504
PS2, Line 504: err_field_incorrectly_set
Correct this to be style-compliant.
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@511
PS2, Line 511: // Copy the non-null value.
: Status row_status;
: RETURN_NOT_OK(ReadColumn(
: col, tablet_row.mutable_cell_ptr(tablet_col_idx), &row_status));
: if (PREDICT_FALSE(!row_status.ok())) {
: op->SetFailureStatusOnce(row_status);
: }
Make this first part of the corresponding if/else for better readability since this one is short, and the other is long.
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@796
PS2, Line 796: /* if (tablet_schema_->has_auto_incrementing()) {
: return Status::NotSupported(
: Substitute("tables with auto-incrementing column do not support "
: "$0 operations", RowOperationsPB_Type_Name(type)));
:
If this code isn't needed, just remove it, don't comment out (especially with such style of commenting).
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/tablet/tablet_auto_incrementing-test.cc
File src/kudu/tablet/tablet_auto_incrementing-test.cc:
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/tablet/tablet_auto_incrementing-test.cc@161
PS2, Line 161: }
It would be great to add some coverage for the following conditions:
* specify a value of the auto-incremeting column for operations that don't support that
* specify an invalid value (e.g., a negative one) for the auto-incrementing column for operations that support setting the value
* specify the maximum allowable value for the auto-incrementing column with UPSERT, and then try to insert one more row without specifying the value for the column
--
To view, visit http://gerrit.cloudera.org:8080/20061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
Gerrit-Change-Number: 20061
Gerrit-PatchSet: 2
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Fri, 16 Jun 2023 18:29:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] [server] KUDU-1945 Auto incrementing column Upsert support
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/20061 )
Change subject: [server] KUDU-1945 Auto_incrementing column Upsert support
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/20061/1/src/kudu/common/row_operations.cc
File src/kudu/common/row_operations.cc:
http://gerrit.cloudera.org:8080/#/c/20061/1/src/kudu/common/row_operations.cc@478
PS1, Line 478: op->type == RowOperationsPB_Type_UPSERT ||
: op->type == RowOperationsPB_Type_UPSERT_IGNORE
> RowOperationsPB_Type_UPSERT means RowOperationsPB_Type_INSERT or RowOperati
At this stage Upsert is still treated as a separate op. Later down the write process this upsert is changed into update or insert depending on the data being written.
https://github.com/apache/kudu/blob/master/src/kudu/tablet/tablet.cc#L848
http://gerrit.cloudera.org:8080/#/c/20061/1/src/kudu/common/row_operations.cc@489
PS1, Line 489: if (counter < 0) {
> warning: The left operand of '<' is a garbage value [clang-analyzer-core.Un
Done
http://gerrit.cloudera.org:8080/#/c/20061/1/src/kudu/common/row_operations.cc@491
PS1, Line 491: "column value");
> nit: alignment
Done
http://gerrit.cloudera.org:8080/#/c/20061/1/src/kudu/common/row_operations.cc@495
PS1, Line 495: // Check if the provided counter value is less than what is in memory
: // and update the counter in memory.
> This auto-incrementing counter is set by the user at client?
For the upsert operation we need the user to present the auto-incrementing column value.
Smaller value is not ignored by the server but it doesn't update the in-memory counter on the server side. As long as the row is not present, the user provided row is written.
--
To view, visit http://gerrit.cloudera.org:8080/20061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
Gerrit-Change-Number: 20061
Gerrit-PatchSet: 1
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Fri, 16 Jun 2023 03:55:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] [server] KUDU-1945 Auto incrementing column Upsert support
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20061 )
Change subject: [server] KUDU-1945 Auto_incrementing column Upsert support
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc
File src/kudu/common/row_operations.cc:
http://gerrit.cloudera.org:8080/#/c/20061/2/src/kudu/common/row_operations.cc@490
PS2, Line 490: err_value
> Correct this to be style-compliant (should be in form cErrValue or somethin
https://google.github.io/styleguide/cppguide.html#Constant_Names
kErrValue
--
To view, visit http://gerrit.cloudera.org:8080/20061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
Gerrit-Change-Number: 20061
Gerrit-PatchSet: 2
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Fri, 16 Jun 2023 21:27:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] [server] KUDU-1945 Auto incrementing column UPSERT support
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Marton Greber, Alexey Serbin, Yuqi Du, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20061
to look at the new patch set (#6).
Change subject: [server] KUDU-1945 Auto_incrementing column UPSERT support
......................................................................
[server] KUDU-1945 Auto_incrementing column UPSERT support
Add UPSERT operation support on the server side when the entire row
is present in the request including the auto-incrementing column.
There will be followup patches to update the clients and test the
corresponding changes as well.
After this patch below is the different operation support for
tables with auto-incrementing columns from the Kudu client.
Operation Presence of auto-incrmeneting column
INSERT/INSERT_IGNORE Should not be specified
UPDATE/UPDATE_IGNORE Has to be specified
UPSERT/UPSERT_IGNORE Has to be specified
DELETE/DELETE_IGNORE Has to be specified
This implemention of UPSERT operation behavior in this patch
is to facilitate incremental restores consistently when using
Kudu backup and restore tool.
More details in the design document:
https://docs.google.com/document/d/1-x6F5TkRYMqt2umDBQ5x-Aa4dqKwFYpbxo1A6QoMafo/edit#heading=h.vnagsa4oxwrf
Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
---
M python/kudu/tests/test_client.py
M src/kudu/common/row_operations.cc
M src/kudu/tablet/tablet_auto_incrementing-test.cc
3 files changed, 172 insertions(+), 50 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/20061/6
--
To view, visit http://gerrit.cloudera.org:8080/20061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
Gerrit-Change-Number: 20061
Gerrit-PatchSet: 6
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [server] KUDU-1945 Auto incrementing column UPSERT support
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Marton Greber, Alexey Serbin, Yuqi Du, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20061
to look at the new patch set (#8).
Change subject: [server] KUDU-1945 Auto_incrementing column UPSERT support
......................................................................
[server] KUDU-1945 Auto_incrementing column UPSERT support
Add UPSERT operation support on the server side when the entire row
is present in the request including the auto-incrementing column.
There will be followup patches to update the clients and test the
corresponding changes as well.
After this patch below is the different operation support for
tables with auto-incrementing columns from the Kudu client.
Operation Presence of auto-incrmeneting column
INSERT/INSERT_IGNORE Should not be specified
UPDATE/UPDATE_IGNORE Has to be specified
UPSERT/UPSERT_IGNORE Has to be specified
DELETE/DELETE_IGNORE Has to be specified
This implemention of UPSERT operation behavior in this patch
is to facilitate incremental restores consistently when using
Kudu backup and restore tool.
More details in the design document:
https://docs.google.com/document/d/1-x6F5TkRYMqt2umDBQ5x-Aa4dqKwFYpbxo1A6QoMafo/edit#heading=h.vnagsa4oxwrf
Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
---
M python/kudu/tests/test_client.py
M src/kudu/common/row_operations.cc
M src/kudu/tablet/tablet_auto_incrementing-test.cc
3 files changed, 172 insertions(+), 50 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/20061/8
--
To view, visit http://gerrit.cloudera.org:8080/20061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
Gerrit-Change-Number: 20061
Gerrit-PatchSet: 8
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [server] KUDU-1945 Auto incrementing column UPSERT support
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Marton Greber, Alexey Serbin, Yuqi Du, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20061
to look at the new patch set (#10).
Change subject: [server] KUDU-1945 Auto_incrementing column UPSERT support
......................................................................
[server] KUDU-1945 Auto_incrementing column UPSERT support
Add UPSERT operation support on the server side when the entire row
is present in the request including the auto-incrementing column.
There will be followup patches to update the clients and test the
corresponding changes as well.
With this patch the expected behavior for different operations
from Kudu clients is as follows:
Operation Presence of auto-incrementing column
INSERT/INSERT_IGNORE Should not be specified
UPDATE/UPDATE_IGNORE Has to be specified
UPSERT/UPSERT_IGNORE Has to be specified
DELETE/DELETE_IGNORE Has to be specified
This implementation of UPSERT operation behavior in this patch
is to facilitate incremental restores consistently when using
Kudu backup and restore tool.
More details in the design document:
https://docs.google.com/document/d/1-x6F5TkRYMqt2umDBQ5x-Aa4dqKwFYpbxo1A6QoMafo/edit#heading=h.vnagsa4oxwrf
Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
---
M python/kudu/tests/test_client.py
M src/kudu/common/row_operations.cc
M src/kudu/tablet/tablet_auto_incrementing-test.cc
3 files changed, 203 insertions(+), 50 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/20061/10
--
To view, visit http://gerrit.cloudera.org:8080/20061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
Gerrit-Change-Number: 20061
Gerrit-PatchSet: 10
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [server] KUDU-1945 Auto incrementing column UPSERT support
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20061 )
Change subject: [server] KUDU-1945 Auto_incrementing column UPSERT support
......................................................................
Patch Set 10: Verified+1
unrelated test failure in DDLDuringRebalancingTest.TablesCreatedAndDeletedDuringRebalancing/0 (RELEASE)
--
To view, visit http://gerrit.cloudera.org:8080/20061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
Gerrit-Change-Number: 20061
Gerrit-PatchSet: 10
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Fri, 23 Jun 2023 20:47:23 +0000
Gerrit-HasComments: No
[kudu-CR] [server] KUDU-1945 Auto incrementing column UPSERT support
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20061 )
Change subject: [server] KUDU-1945 Auto_incrementing column UPSERT support
......................................................................
Patch Set 10: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/20061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
Gerrit-Change-Number: 20061
Gerrit-PatchSet: 10
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Fri, 23 Jun 2023 20:46:57 +0000
Gerrit-HasComments: No
[kudu-CR] [server] KUDU-1945 Auto incrementing column UPSERT support
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20061 )
Change subject: [server] KUDU-1945 Auto_incrementing column UPSERT support
......................................................................
Patch Set 8: Code-Review+1
(4 comments)
Overall looks good, just a few nits and a request to add a few more test sub-cases.
http://gerrit.cloudera.org:8080/#/c/20061/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20061/6//COMMIT_MSG@14
PS6, Line 14: After this patch below is the different operation support for
: tables with auto-incrementing columns from the Kudu client.
This is hard to parse, please update this to be more readable.
http://gerrit.cloudera.org:8080/#/c/20061/6//COMMIT_MSG@23
PS6, Line 23: implemention
implementation
http://gerrit.cloudera.org:8080/#/c/20061/8/src/kudu/tablet/tablet_auto_incrementing-test.cc
File src/kudu/tablet/tablet_auto_incrementing-test.cc:
http://gerrit.cloudera.org:8080/#/c/20061/8/src/kudu/tablet/tablet_auto_incrementing-test.cc@19
PS8, Line 19:
nit: remove empty line 19
http://gerrit.cloudera.org:8080/#/c/20061/8/src/kudu/tablet/tablet_auto_incrementing-test.cc@189
PS8, Line 189: {
: unique_ptr<KuduPartialRow> row(new KuduPartialRow(&client_schema_));
: ASSERT_OK(row->SetInt64(0, INT64_MAX));
: ASSERT_OK(row->SetInt32(1, 1337));
: ASSERT_OK(writer_->Upsert(*row));
: }
Thank you for adding these negative scenarios!
I'm curious: what's the expected behavior if after this UPSERT with INT64_MAX for the auto-incrementing column's value another UPSERT attempted with value that's not yet present in the table? I guess that should succeed, right? Also, attempting another UPSERT with INT64_MAX after failed INSERT should succeed as well, correct?
Does it make sense to add a sub-case for that as well?
I guess it's important to splice them like the following to test how the counter is updated at the server side:
UPSERT(INT64_MAX)
UPSERT(not-yet-existing-value-for-auto-increment-column)
INSERT
UPSERT(INT64_MAX)
--
To view, visit http://gerrit.cloudera.org:8080/20061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
Gerrit-Change-Number: 20061
Gerrit-PatchSet: 8
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Fri, 23 Jun 2023 06:12:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] [server] KUDU-1945 Auto incrementing column Upsert support
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/20061 )
Change subject: [server] KUDU-1945 Auto_incrementing column Upsert support
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/20061/1/src/kudu/common/row_operations.cc
File src/kudu/common/row_operations.cc:
http://gerrit.cloudera.org:8080/#/c/20061/1/src/kudu/common/row_operations.cc@478
PS1, Line 478: op->type == RowOperationsPB_Type_UPSERT ||
: op->type == RowOperationsPB_Type_UPSERT_IGNORE
RowOperationsPB_Type_UPSERT means RowOperationsPB_Type_INSERT or RowOperationsPB_Type_UPDATE,
according to context, I guess it is for RowOperationsPB_Type_UPDATE. So should we add RowOperationsPB_Type_UPDATE at this?
http://gerrit.cloudera.org:8080/#/c/20061/1/src/kudu/common/row_operations.cc@491
PS1, Line 491: "column value");
nit: alignment
http://gerrit.cloudera.org:8080/#/c/20061/1/src/kudu/common/row_operations.cc@495
PS1, Line 495: // Check if the provided counter value is less than what is in memory
: // and update the counter in memory.
This auto-incrementing counter is set by the user at client?
If a user set a smaller value, server side will ignore this set and change it to a bigger one using an obscure way for users. In kudu, this is correct, but for users it seems not good enough.
Whether it is necessary to provide more response messages or return not ok?
--
To view, visit http://gerrit.cloudera.org:8080/20061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5cc4d80f77c165452572948f68c76fc70394d47
Gerrit-Change-Number: 20061
Gerrit-PatchSet: 1
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Wed, 14 Jun 2023 08:07:02 +0000
Gerrit-HasComments: Yes