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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Wed, 17 Jul 2019 05:23:44 +0000 Gerrit-HasComments: Yes
