Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18742 )
Change subject: KUDU-3353 [schema] Add an immutable attribute on column schema (part 1) ...................................................................... Patch Set 2: (13 comments) http://gerrit.cloudera.org:8080/#/c/18742/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18742/2//COMMIT_MSG@13 PS2, Line 13: UPDATE_IGNORE Please document what sort of errors are now possible from UPDATE_IGNORE operations after this update. In that case, is it possible to tell from an absent row error and present row, but an attempt to update an immutable cell? http://gerrit.cloudera.org:8080/#/c/18742/2//COMMIT_MSG@15 PS2, Line 15: It would be great to add examples of use cases that benefit or require this feature. http://gerrit.cloudera.org:8080/#/c/18742/2//COMMIT_MSG@17 PS2, Line 17: neccessary necessary http://gerrit.cloudera.org:8080/#/c/18742/2/src/kudu/common/row_operations.h File src/kudu/common/row_operations.h: http://gerrit.cloudera.org:8080/#/c/18742/2/src/kudu/common/row_operations.h@96 PS2, Line 96: an error Which error? Any error or only specific ones? If there were multiple immutable columns in the row, how to tell for which update errors were ignored? http://gerrit.cloudera.org:8080/#/c/18742/2/src/kudu/common/row_operations.cc File src/kudu/common/row_operations.cc: http://gerrit.cloudera.org:8080/#/c/18742/2/src/kudu/common/row_operations.cc@73 PS2, Line 73: return "UPSERT IGNORE" + schema.DebugRow(ConstContiguousRow(&schema, row_data)); nit: missing space after 'IGNORE' http://gerrit.cloudera.org:8080/#/c/18742/2/src/kudu/common/row_operations.cc@574 PS2, Line 574: op->error_ignored = true; nit: maybe, add DCHECK_EQ(RowOperationsPB::UPDATE_IGNORE, op->type) under 'else' ? http://gerrit.cloudera.org:8080/#/c/18742/2/src/kudu/common/schema.h File src/kudu/common/schema.h: http://gerrit.cloudera.org:8080/#/c/18742/2/src/kudu/common/schema.h@212 PS2, Line 212: Immutable column means the cell value can not be updated after the first insert. Is it possible to set the 'immutable' flag on a key column? If yes, what sort of errors are expected when trying to update a pk key cell with UPDATE_IGNORE? http://gerrit.cloudera.org:8080/#/c/18742/2/src/kudu/consensus/log-test.cc File src/kudu/consensus/log-test.cc: http://gerrit.cloudera.org:8080/#/c/18742/2/src/kudu/consensus/log-test.cc@1076 PS2, Line 1076: 337 Why is this change? http://gerrit.cloudera.org:8080/#/c/18742/2/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/18742/2/src/kudu/master/master.proto@1125 PS2, Line 1125: UPSERT_IGNORE Since there's been three official releases of Kudu with IGNORE_OPERATIONS feature that didn't include UPSERT_IGNORE (1.14, 1.15, 1.16), I guess it's necessary to add an extra flag to track the support of UPSERT_IGNORE? http://gerrit.cloudera.org:8080/#/c/18742/2/src/kudu/tablet/tablet-test.cc File src/kudu/tablet/tablet-test.cc: http://gerrit.cloudera.org:8080/#/c/18742/2/src/kudu/tablet/tablet-test.cc@921 PS2, Line 921: } Please add a test that shows how UPDATE_IGNORE behaves in the following cases: * presence of the target row and a presence of at least one immutable to update * absence of the target row but a present of columns with immutable attribute http://gerrit.cloudera.org:8080/#/c/18742/2/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/18742/2/src/kudu/tablet/tablet.cc@851 PS2, Line 851: auto op_type = upsert->decoded_op.type; nit: add 'const'? http://gerrit.cloudera.org:8080/#/c/18742/2/src/kudu/tablet/tablet_metrics.h File src/kudu/tablet/tablet_metrics.h: http://gerrit.cloudera.org:8080/#/c/18742/2/src/kudu/tablet/tablet_metrics.h@52 PS2, Line 52: scoped_refptr<Counter> upsert_ignore_errors; Does it makes sense to add metrics for the new operation to track number successful UPSERT_IGNORE operations processed into ResourceMetricsPB structure in tserser.proto as well and update corresponding code paths? http://gerrit.cloudera.org:8080/#/c/18742/2/src/kudu/util/status.h File src/kudu/util/status.h: http://gerrit.cloudera.org:8080/#/c/18742/2/src/kudu/util/status.h@471 PS2, Line 471: // kCancelled = 19, Could you add a comment to explain why to reserve this enumeration descriptor (I guess that stems from CANCELLED enum descriptor in wire_protocol.proto)? -- To view, visit http://gerrit.cloudera.org:8080/18742 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I01e5a806c0e873239b49e6d0b37a7e36578b508d Gerrit-Change-Number: 18742 Gerrit-PatchSet: 2 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Thu, 28 Jul 2022 02:17:16 +0000 Gerrit-HasComments: Yes
