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 <[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: Sun, 21 Jul 2019 10:15:15 +0000 Gerrit-HasComments: Yes
