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 <[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: Thu, 25 Jul 2019 09:13:56 +0000 Gerrit-HasComments: Yes
