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 <[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: Mon, 15 Jul 2019 23:34:38 +0000 Gerrit-HasComments: Yes
