Yingchun Lai 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: (12 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 ope You meant possible errors from 'UPDATE' operations? Added it now. 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 Done http://gerrit.cloudera.org:8080/#/c/18742/2//COMMIT_MSG@17 PS2, Line 17: neccessary > necessary Done 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? Added more comments. If there were multiple immutable columns in the row, only the first column name will be in the error hint message. (It is the way how non-nullable columns do [1]). 1. https://github.com/apache/kudu/blob/master/src/kudu/common/row_operations.cc#L576 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' Done 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 ' Done 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 s The primary key is not allowed to be updated in Kudu, UPDATE_IGNORE is act as an update on another row corresponding to the 'new' primary key. We didn't forbiden to set the 'immutable' flag on a key column, but it's meanless and no effect. 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 f You're right, done. 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 cas Done 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'? Done 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 su Good advice, done. 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 descript Done -- 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: Fri, 29 Jul 2022 17:46:59 +0000 Gerrit-HasComments: Yes
