You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Yingchun Lai (Code Review)" <ge...@cloudera.org> on 2019/07/15 11:01:06 UTC
[kudu-CR] KUDU-2625: Support per-row error check in prepare stage
Yingchun Lai has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13864
Change subject: KUDU-2625: Support per-row error check in prepare stage
......................................................................
KUDU-2625: Support per-row error check in prepare stage
Tablet servers reject the whole batch if there is a row that violates
table schema constraints (e.g., presence of null values for non-nullable
columns). This behavior is different from the case when errors happen at
later stage of 'applying' received write operations (e.g., a duplicate
key error).
This patch reject only the 'bad' rows instead of the whole batch when
checked errors in prepare stage.
Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0
---
M src/kudu/common/partial_row.h
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/row_operations.h
M src/kudu/tablet/row_op.cc
M src/kudu/tablet/row_op.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/write_transaction.cc
9 files changed, 229 insertions(+), 159 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/13864/1
--
To view, visit http://gerrit.cloudera.org:8080/13864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0
Gerrit-Change-Number: 13864
Gerrit-PatchSet: 1
Gerrit-Owner: Yingchun Lai <40...@qq.com>
[kudu-CR] KUDU-2625: Support per-row error check in prepare stage
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/13864 )
Change subject: KUDU-2625: Support per-row error check in prepare stage
......................................................................
Patch Set 6:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13864/5/src/kudu/common/row_operations.h
File src/kudu/common/row_operations.h:
http://gerrit.cloudera.org:8080/#/c/13864/5/src/kudu/common/row_operations.h@113
PS5, Line 113: Status ReadColumnAndDiscard(const ColumnSchema& col);
> Nit: maybe call it ReadColumnAndDiscard instead?
Done
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/common/row_operations.cc
File src/kudu/common/row_operations.cc:
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/common/row_operations.cc@616
PS3, Line 616: ops->push_back(op);
: }
:
> But the per-row errors still propagate through Apply and end up returned to
Yes
http://gerrit.cloudera.org:8080/#/c/13864/5/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/13864/5/src/kudu/tablet/tablet.cc@461
PS5, Line 461: if (op->has_result()) continue;
> Just so I understand, this is an optimization in that we're not going to ap
Yes
--
To view, visit http://gerrit.cloudera.org:8080/13864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0
Gerrit-Change-Number: 13864
Gerrit-PatchSet: 6
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Fri, 26 Jul 2019 00:02:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2625: Support per-row error check in prepare stage
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13864 )
Change subject: KUDU-2625: Support per-row error check in prepare stage
......................................................................
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13864/5/src/kudu/common/row_operations.h
File src/kudu/common/row_operations.h:
http://gerrit.cloudera.org:8080/#/c/13864/5/src/kudu/common/row_operations.h@113
PS5, Line 113: Status ConsumeUselessColumn(const ColumnSchema& col);
Nit: maybe call it ReadColumnAndDiscard instead?
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/common/row_operations.cc
File src/kudu/common/row_operations.cc:
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/common/row_operations.cc@616
PS3, Line 616: ops->push_back(op);
: }
:
> I've removed these code, since per-row errors not supported when return fro
But the per-row errors still propagate through Apply and end up returned to the client, right?
http://gerrit.cloudera.org:8080/#/c/13864/5/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/13864/5/src/kudu/tablet/tablet.cc@461
PS5, Line 461: if (op->has_result()) continue;
Just so I understand, this is an optimization in that we're not going to apply that op (because it failed), so there's no reason to take a lock on its primary key?
--
To view, visit http://gerrit.cloudera.org:8080/13864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0
Gerrit-Change-Number: 13864
Gerrit-PatchSet: 5
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Thu, 25 Jul 2019 18:43:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2625: Support per-row error check in prepare stage
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/13864 )
Change subject: KUDU-2625: Support per-row error check in prepare stage
......................................................................
Patch Set 2:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/13864/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13864/1//COMMIT_MSG@14
PS1, Line 14: This patch reject only the 'bad' rows instead of the whole batch when
> +1
Added some test cases in client/client-test.cc
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations-test.cc
File src/kudu/common/row_operations-test.cc:
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations-test.cc@554
PS1, Line 554: TEST_F(
> Any particular reason not to rely on compiler's judgment whether to inline
No more reason, I just copy it from common/partial_row.cc. I'll remove it.
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations-test.cc@554
PS1, Line 554: TEST_F(
> Agreed, especially for a test function.
Done
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.h
File src/kudu/common/row_operations.h:
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.h@20
PS1, Line 20: #include <cstdint>
> nit: drop the empty line
Done
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.h@82
PS1, Line 82: };
> To clarify, "The 'result' member will only be updated the first time this f
Done
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc
File src/kudu/common/row_operations.cc:
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc@84
PS1, Line 84: DCHECK(!s.ok());
> You could DCHECK(!s.ok()); all callers respect that invariant.
Done
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc@395
PS1, Line 395: ust
> Nit: not your fault, but add an end parens here.
Done
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc@409
PS1, Line 409:
> The control flow here is tricky. Would it be possible to apply the same rul
Each SetFailureStatusOnce has different following flow, seems SCOPED_CLEANUP is not suitable here.
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc@448
PS1, Line 448:
> Given the current semantics of the SetFailureStatusOnce(), I'm not sure it
Because a row has several columns, we have to consume the following cells of this row (by ReadColumn in L458), otherwise, the following rows of the same request could not be read normally. And for this cell, it is not set, so we 'continue' here and not to ReadColumn. On the other hand, on L454, there is no 'continue', because the cell is set (a NULL value), we have to read the cell by ReadColumn.
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc@454
PS1, Line 454: if (PREDICT_FALSE(client_set_to_null)) {
: op->SetFailureStatusOnce(Status::InvalidArgument("NULL values not
> Shouldn't we break or continue after this?
The same as above.
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc@491
PS1, Line 491: "NULL
> Why to continue if SetFailureStatusOnce() is not going to update the status
The same as above.
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc@500
PS1, Line 500: // No actual column updates specified!
: op->SetFailureStatusOnce(Status::InvalidArgument("No fields updated, key is",
> Why don't we return after this, to avoid the extra nesting on L504-513?
The same as above.
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc@521
PS1, Line 521: const ColumnSchema& col = tablet_schema_->column(tablet_col_idx);
> The same here: why to continue if the op result status is not going to be u
The same, just to consume input stream.
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc@572
PS1, Line 572:
> Nit: there's an unnecessary space here.
Done
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/tablet/row_op.h
File src/kudu/tablet/row_op.h:
PS1:
> It seems changes in this file are unrelated to the essence of this patch, r
Done
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/tablet/row_op.h@71
PS1, Line 71: DecodedRowOperation decoded_op;
> FWIW, the existing non-underscore naming reflects the fact that this is a s
Reverted, and I will remove the extra '_' for 'orig_result_from_log_'.
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/tablet/row_op.cc
File src/kudu/tablet/row_op.cc:
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/tablet/row_op.cc@41
PS1, Line 41: validated = true;
> Should add a comment here explaining why we're doing this.
Done
--
To view, visit http://gerrit.cloudera.org:8080/13864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0
Gerrit-Change-Number: 13864
Gerrit-PatchSet: 2
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Sun, 21 Jul 2019 10:15:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2625: Support per-row error check in prepare stage
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13864 )
Change subject: KUDU-2625: Support per-row error check in prepare stage
......................................................................
Patch Set 3:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/client/client-test.cc@2732
PS3, Line 2732: ASSERT_EQ(R"((int32 key=1, int32 int_val=1, string string_val="One", int32 non_null_with_default=12345))", rows[0]);
: ASSERT_EQ(R"((int32 key=2, int32 int_val=22, string string_val="Two", int32 non_null_with_default=12345))", rows[1]);
These lines look too long, could you wrap?
Below too.
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/client/client-test.cc@2789
PS3, Line 2789: CHECK(!overflow);
: CHECK_EQ(2, errors.size());
Use ASSERTs here.
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/client/client-test.cc@2798
PS3, Line 2798: ScanTableToStrings(client_table_.get(), &rows);
Need to wrap these calls in NO_FATALS.
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/common/row_operations.cc
File src/kudu/common/row_operations.cc:
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/common/row_operations.cc@449
PS3, Line 449: continue;
Looking over DecodeUpdateOrDelete, it looks like the "nothing to consume, move on to the next column" case is less common than the "we need to consume the value in order to properly validate subsequent rows" case. As such, could you add a comment here explaining why we're avoiding ReadColumn?
Alternatively, you could augment "First process the key columns" to explain the overall flow and help readers understand why, in the event of a key column validation failure, we can't just short circuit out.
We also need equivalent breadcrumbs in the comments below, dealing with non-key columns.
Finally, could you add some multi-row tests to row_operations-test, where a validation error in the first row doesn't mess up the subsequent rows? Or do we have such coverage already?
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/common/row_operations.cc@527
PS3, Line 527: ReadColumn(col, scratch);
Wrap in RETURN_NOT_OK? Or should this be wrapped in ignore_result?
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/common/row_operations.cc@616
PS3, Line 616: if (!ops->empty() && all_error) {
: return Status::InvalidArgument("all row operations decoded error");
: }
Why this short-circuit? Shouldn't we surface a batch of 10/10 decoding errors using per-row errors?
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/tablet/transactions/write_transaction.cc
File src/kudu/tablet/transactions/write_transaction.cc:
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/tablet/transactions/write_transaction.cc@169
PS3, Line 169: UpdatePerRowErrors();
Is this here just to accommodate the new "if all ops failed to decode, fail the entire write" codepath?
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/tablet/transactions/write_transaction.cc@220
PS3, Line 220: i++;
Nit: not your fault (since you're just moving code), but this would probably be clearer if you used a for loop iterating over i and set up a local const RowOp* in the body of the loop.
--
To view, visit http://gerrit.cloudera.org:8080/13864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0
Gerrit-Change-Number: 13864
Gerrit-PatchSet: 3
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Mon, 22 Jul 2019 21:25:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2625: Support per-row error check in prepare stage
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13864
to look at the new patch set (#3).
Change subject: KUDU-2625: Support per-row error check in prepare stage
......................................................................
KUDU-2625: Support per-row error check in prepare stage
Tablet servers reject the whole batch if there is a row that violates
table schema constraints (e.g., presence of null values for non-nullable
columns). This behavior is different from the case when errors happen at
later stage of 'applying' received write operations (e.g., a duplicate
key error).
This patch reject only the 'bad' rows instead of the whole batch when
checked errors in prepare stage.
Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0
---
M src/kudu/client/client-test.cc
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/row_operations.h
M src/kudu/common/schema.h
M src/kudu/tablet/row_op.cc
M src/kudu/tablet/row_op.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tablet/transactions/write_transaction.h
10 files changed, 323 insertions(+), 114 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/13864/3
--
To view, visit http://gerrit.cloudera.org:8080/13864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0
Gerrit-Change-Number: 13864
Gerrit-PatchSet: 3
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] KUDU-2625: Support per-row error check in prepare stage
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13864 )
Change subject: KUDU-2625: Support per-row error check in prepare stage
......................................................................
KUDU-2625: Support per-row error check in prepare stage
Tablet servers reject the whole batch if there is a row that violates
table schema constraints (e.g., presence of null values for non-nullable
columns). This behavior is different from the case when errors happen at
later stage of 'applying' received write operations (e.g., a duplicate
key error).
This patch reject only the 'bad' rows instead of the whole batch when
checked errors in prepare stage.
Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0
Reviewed-on: http://gerrit.cloudera.org:8080/13864
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/client/client-test.cc
M src/kudu/common/partial_row.h
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/row_operations.h
M src/kudu/common/schema.h
M src/kudu/tablet/row_op.cc
M src/kudu/tablet/row_op.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tablet/transactions/write_transaction.h
11 files changed, 441 insertions(+), 122 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/13864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0
Gerrit-Change-Number: 13864
Gerrit-PatchSet: 7
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] KUDU-2625: Support per-row error check in prepare stage
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13864
to look at the new patch set (#5).
Change subject: KUDU-2625: Support per-row error check in prepare stage
......................................................................
KUDU-2625: Support per-row error check in prepare stage
Tablet servers reject the whole batch if there is a row that violates
table schema constraints (e.g., presence of null values for non-nullable
columns). This behavior is different from the case when errors happen at
later stage of 'applying' received write operations (e.g., a duplicate
key error).
This patch reject only the 'bad' rows instead of the whole batch when
checked errors in prepare stage.
Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0
---
M src/kudu/client/client-test.cc
M src/kudu/common/partial_row.h
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/row_operations.h
M src/kudu/common/schema.h
M src/kudu/tablet/row_op.cc
M src/kudu/tablet/row_op.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tablet/transactions/write_transaction.h
11 files changed, 441 insertions(+), 122 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/13864/5
--
To view, visit http://gerrit.cloudera.org:8080/13864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0
Gerrit-Change-Number: 13864
Gerrit-PatchSet: 5
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] KUDU-2625: Support per-row error check in prepare stage
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13864 )
Change subject: KUDU-2625: Support per-row error check in prepare stage
......................................................................
Patch Set 1:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/13864/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13864/1//COMMIT_MSG@14
PS1, Line 14: This patch reject only the 'bad' rows instead of the whole batch when
> It would be nice to have and-to-end test where it's verified that the desir
+1
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations-test.cc
File src/kudu/common/row_operations-test.cc:
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations-test.cc@554
PS1, Line 554: inline
> Any particular reason not to rely on compiler's judgment whether to inline
Agreed, especially for a test function.
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.h
File src/kudu/common/row_operations.h:
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.h@82
PS1, Line 82: // Status will be updated only once when 's' is not Status::OK().
To clarify, "The 'result' member will only be updated the first time this function is called."
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc
File src/kudu/common/row_operations.cc:
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc@84
PS1, Line 84: if (result.ok() && !s.ok()) {
You could DCHECK(!s.ok()); all callers respect that invariant.
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc@395
PS1, Line 395: row
Nit: not your fault, but add an end parens here.
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc@409
PS1, Line 409: Status RowOperationsPBDecoder::DecodeUpdateOrDelete(const ClientServerMapping& mapping,
The control flow here is tricky. Would it be possible to apply the same rule whenever we call SetFailureStatusOnce (i.e. return early, or break, or something like that)? If there's some code you always want to run when returning from the function, you could wrap it in a SCOPED_CLEANUP to avoid duplicating it.
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc@454
PS1, Line 454: op->SetFailureStatusOnce(Status::InvalidArgument("NULL values not allowed for key column",
: col.ToString()));
Shouldn't we break or continue after this?
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc@500
PS1, Line 500: op->SetFailureStatusOnce(Status::InvalidArgument("No fields updated, key is",
: tablet_schema_->DebugRowKey(rowkey)));
Why don't we return after this, to avoid the extra nesting on L504-513?
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc@572
PS1, Line 572: >
Nit: there's an unnecessary space here.
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/tablet/row_op.h
File src/kudu/tablet/row_op.h:
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/tablet/row_op.h@71
PS1, Line 71: DecodedRowOperation decoded_op_;
FWIW, the existing non-underscore naming reflects the fact that this is a struct and these members are public.
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/tablet/row_op.cc
File src/kudu/tablet/row_op.cc:
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/tablet/row_op.cc@41
PS1, Line 41: validated_ = true;
Should add a comment here explaining why we're doing this.
--
To view, visit http://gerrit.cloudera.org:8080/13864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0
Gerrit-Change-Number: 13864
Gerrit-PatchSet: 1
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Wed, 17 Jul 2019 05:23:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2625: Support per-row error check in prepare stage
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13864 )
Change subject: KUDU-2625: Support per-row error check in prepare stage
......................................................................
Patch Set 7: Code-Review+2
> Alexey do you want to take another look?
I'm late, but this LGTM. Thank you Yingchun and Adar!
--
To view, visit http://gerrit.cloudera.org:8080/13864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0
Gerrit-Change-Number: 13864
Gerrit-PatchSet: 7
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Fri, 26 Jul 2019 16:43:31 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2625: Support per-row error check in prepare stage
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/13864 )
Change subject: KUDU-2625: Support per-row error check in prepare stage
......................................................................
Patch Set 4:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/client/client-test.cc@2732
PS3, Line 2732: ASSERT_EQ(error->status().ToString(),
: "Invalid argument: No fields updated, key is: (int32 key=1)");
> These lines look too long, could you wrap?
Done
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/client/client-test.cc@2789
PS3, Line 2789: ASSERT_OK(ApplyInsertToSession(session.get(), client_table_, 1, 1, "One"));
: ASSERT_OK(ApplyInsertToSess
> Use ASSERTs here.
Done
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/client/client-test.cc@2798
PS3, Line 2798: ASSERT_STR_CONTAINS(s.ToString(), "Some errors occurred");
> Need to wrap these calls in NO_FATALS.
Done
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/common/row_operations.cc
File src/kudu/common/row_operations.cc:
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/common/row_operations.cc@449
PS3, Line 449: // etc.
> Looking over DecodeUpdateOrDelete, it looks like the "nothing to consume, m
I've wrap some code in ConsumeUselessColumn, the function name is self-explanatory, and I've added some comments in function declare place.
It's not needed to add multi-row tests, because when a row doesn't properly validated (lack some data or left some data), DecodeOperations will return another error, we can detect this error in single row test in row_operations-test.
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/common/row_operations.cc@527
PS3, Line 527: op->SetFailureStatusOnce(Status::InvalidArgument(
> Wrap in RETURN_NOT_OK? Or should this be wrapped in ignore_result?
should be wrapped in RETURN_NOT_OK, Done.
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/common/row_operations.cc@616
PS3, Line 616: ops->push_back(op);
: }
:
> Why this short-circuit? Shouldn't we surface a batch of 10/10 decoding erro
I've removed these code, since per-row errors not supported when return from prepare phase. Maybe I will add it in another patch.
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/tablet/transactions/write_transaction.cc
File src/kudu/tablet/transactions/write_transaction.cc:
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/tablet/transactions/write_transaction.cc@169
PS3, Line 169: // TODO(unknown): is MISMATCHED_SCHEMA always right here? probably not.
> Is this here just to accommodate the new "if all ops failed to decode, fail
removed.
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/tablet/transactions/write_transaction.cc@220
PS3, Line 220: }
> Nit: not your fault (since you're just moving code), but this would probabl
Done
--
To view, visit http://gerrit.cloudera.org:8080/13864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0
Gerrit-Change-Number: 13864
Gerrit-PatchSet: 4
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Thu, 25 Jul 2019 09:13:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2625: Support per-row error check in prepare stage
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13864
to look at the new patch set (#2).
Change subject: KUDU-2625: Support per-row error check in prepare stage
......................................................................
KUDU-2625: Support per-row error check in prepare stage
Tablet servers reject the whole batch if there is a row that violates
table schema constraints (e.g., presence of null values for non-nullable
columns). This behavior is different from the case when errors happen at
later stage of 'applying' received write operations (e.g., a duplicate
key error).
This patch reject only the 'bad' rows instead of the whole batch when
checked errors in prepare stage.
Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0
---
M src/kudu/client/client-test.cc
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/row_operations.h
M src/kudu/common/schema.h
M src/kudu/tablet/row_op.cc
M src/kudu/tablet/row_op.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tablet/transactions/write_transaction.h
10 files changed, 323 insertions(+), 114 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/13864/2
--
To view, visit http://gerrit.cloudera.org:8080/13864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0
Gerrit-Change-Number: 13864
Gerrit-PatchSet: 2
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] KUDU-2625: Support per-row error check in prepare stage
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13864
to look at the new patch set (#4).
Change subject: KUDU-2625: Support per-row error check in prepare stage
......................................................................
KUDU-2625: Support per-row error check in prepare stage
Tablet servers reject the whole batch if there is a row that violates
table schema constraints (e.g., presence of null values for non-nullable
columns). This behavior is different from the case when errors happen at
later stage of 'applying' received write operations (e.g., a duplicate
key error).
This patch reject only the 'bad' rows instead of the whole batch when
checked errors in prepare stage.
Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0
---
M src/kudu/client/client-test.cc
M src/kudu/common/partial_row.h
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/row_operations.h
M src/kudu/common/schema.h
M src/kudu/tablet/row_op.cc
M src/kudu/tablet/row_op.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tablet/transactions/write_transaction.h
11 files changed, 441 insertions(+), 120 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/13864/4
--
To view, visit http://gerrit.cloudera.org:8080/13864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0
Gerrit-Change-Number: 13864
Gerrit-PatchSet: 4
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] KUDU-2625: Support per-row error check in prepare stage
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13864 )
Change subject: KUDU-2625: Support per-row error check in prepare stage
......................................................................
Patch Set 6: Code-Review+2
Alexey do you want to take another look?
--
To view, visit http://gerrit.cloudera.org:8080/13864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0
Gerrit-Change-Number: 13864
Gerrit-PatchSet: 6
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Fri, 26 Jul 2019 00:07:07 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2625: Support per-row error check in prepare stage
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13864
to look at the new patch set (#6).
Change subject: KUDU-2625: Support per-row error check in prepare stage
......................................................................
KUDU-2625: Support per-row error check in prepare stage
Tablet servers reject the whole batch if there is a row that violates
table schema constraints (e.g., presence of null values for non-nullable
columns). This behavior is different from the case when errors happen at
later stage of 'applying' received write operations (e.g., a duplicate
key error).
This patch reject only the 'bad' rows instead of the whole batch when
checked errors in prepare stage.
Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0
---
M src/kudu/client/client-test.cc
M src/kudu/common/partial_row.h
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/row_operations.h
M src/kudu/common/schema.h
M src/kudu/tablet/row_op.cc
M src/kudu/tablet/row_op.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tablet/transactions/write_transaction.h
11 files changed, 441 insertions(+), 122 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/13864/6
--
To view, visit http://gerrit.cloudera.org:8080/13864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0
Gerrit-Change-Number: 13864
Gerrit-PatchSet: 6
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] KUDU-2625: Support per-row error check in prepare stage
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13864 )
Change subject: KUDU-2625: Support per-row error check in prepare stage
......................................................................
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/13864/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13864/1//COMMIT_MSG@14
PS1, Line 14: This patch reject only the 'bad' rows instead of the whole batch when
It would be nice to have and-to-end test where it's verified that the desired result is seen at the client side. The invalid rows should be rejected, and information on the rejected rows should be propagated back to client (at least that's possible now for the Java client, as I understand).
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations-test.cc
File src/kudu/common/row_operations-test.cc:
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations-test.cc@554
PS1, Line 554: inline
Any particular reason not to rely on compiler's judgment whether to inline or not this function? I.e., why 'inline' is necessary here?
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.h
File src/kudu/common/row_operations.h:
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.h@20
PS1, Line 20:
nit: drop the empty line
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc
File src/kudu/common/row_operations.cc:
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc@448
PS1, Line 448: continue;
Given the current semantics of the SetFailureStatusOnce(), I'm not sure it makes sense to continue iterating over other columns: the other errors are not going to affect the status already set. Why not just break out of the cycle here?
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc@491
PS1, Line 491: continue;
Why to continue if SetFailureStatusOnce() is not going to update the status of the operations for any further column?
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc@521
PS1, Line 521: op->SetFailureStatusOnce(Status::InvalidArgument(
The same here: why to continue if the op result status is not going to be updated after the first failure anyways?
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/tablet/row_op.h
File src/kudu/tablet/row_op.h:
PS1:
It seems changes in this file are unrelated to the essence of this patch, right? I think it's better to keep this file unchanged for the sake of clarity, and then update this file in a separate changelist.
--
To view, visit http://gerrit.cloudera.org:8080/13864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0
Gerrit-Change-Number: 13864
Gerrit-PatchSet: 1
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Mon, 15 Jul 2019 23:34:38 +0000
Gerrit-HasComments: Yes