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 7: (20 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: PDATE or UPSE > You meant possible errors from 'UPDATE' operations? I meant whether it's possible to tell between two different error conditions that UPDATE_IGNORE operation now ignores. Yifan's comment on PS7 summarized that pretty well. http://gerrit.cloudera.org:8080/#/c/18742/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18742/7//COMMIT_MSG@10 PS7, Line 10: Add a column attribute to define a column as IMMUTABLE Add a new column attribute IMMUTABLE http://gerrit.cloudera.org:8080/#/c/18742/7//COMMIT_MSG@10 PS7, Line 10: means meaning http://gerrit.cloudera.org:8080/#/c/18742/7//COMMIT_MSG@13 PS7, Line 13: If UPDATE or UPSERT operations are operated on immutable cells : of a present row, a new added error of Status::IsImmutable() : will be returned. How about: An attempt to modify an immutable cell of an existing row by UPDATE or UPSERT operation results in returning the newly added Status::IsImmutable(). http://gerrit.cloudera.org:8080/#/c/18742/7//COMMIT_MSG@16 PS7, Line 16: UPDATE_IGNORE > Yes, you're right. Now UPDATE_IGNORE ops will ignore both of the two errors Thanks for clarifying! Could you add that into the commit description as well, please? Something like With this change, UPDATE_IGNORE ops ignore both 'key not found' and 'update on immutable column' errors. http://gerrit.cloudera.org:8080/#/c/18742/7//COMMIT_MSG@17 PS7, Line 17: update-errors update errors http://gerrit.cloudera.org:8080/#/c/18742/7//COMMIT_MSG@17 PS7, Line 17: Note that : the rows are still upsert/updated but only ignore to update : the immutable cells Note that the rest of the columns are updated accordingly to the operation's data, only the immutable columns aren't changed. http://gerrit.cloudera.org:8080/#/c/18742/7//COMMIT_MSG@22 PS7, Line 22: Some columns are impossible to be updated, but it may present with : different values in every input data flow. E.g. a column of : 'first_login_timestamp', and present as 'login_timestamp' in data : flow. How about: A column represents a semantically constant entity. The corresponding value is present in every row for a particular primary key and might change, but it's captured upon the very first occurrence. An example is 'first_login_timestamp' for a particular user while 'login_timestamp' is present in every login record. http://gerrit.cloudera.org:8080/#/c/18742/7//COMMIT_MSG@26 PS7, Line 26: Some columns are impossible to be updated, and it may present with : the same values in every input data flow. We want to reduce the : length of a column's change list. E.g. a column of 'birthday'. How about: Similar to the item 1, but the corresponding value, if present, is the same with every record for a particular primary key. Here the intention is to reduce the length of the column's change list. An example is a 'birthday' column. http://gerrit.cloudera.org:8080/#/c/18742/7//COMMIT_MSG@30 PS7, Line 30: on server side on the server side http://gerrit.cloudera.org:8080/#/c/18742/7//COMMIT_MSG@30 PS7, Line 30: some : necessary changes necessary changes on the client side http://gerrit.cloudera.org:8080/#/c/18742/7/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java: http://gerrit.cloudera.org:8080/#/c/18742/7/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2720 PS7, Line 2720: // TODO(yingchun): also need add 'public Deferred<Boolean> supportsUpsertIgnoreOperations()' Do you plan to implement that in this patch or in a separate one? http://gerrit.cloudera.org:8080/#/c/18742/7/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduSession.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduSession.java: http://gerrit.cloudera.org:8080/#/c/18742/7/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduSession.java@359 PS7, Line 359: // TODO(yingchun): should test upsert_ignore_errors Do you plan to implement that in this patch or in a separate one? http://gerrit.cloudera.org:8080/#/c/18742/7/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/18742/7/src/kudu/client/client-test.cc@2951 PS7, Line 2951: // TODO(yingchun): should test upsert_ignore_errors Do you plan to implement that in this patch or in a separate one? http://gerrit.cloudera.org:8080/#/c/18742/7/src/kudu/common/row_operations.h File src/kudu/common/row_operations.h: http://gerrit.cloudera.org:8080/#/c/18742/7/src/kudu/common/row_operations.h@97 PS7, Line 97: Now it's possible to be: How about: As of now, the error could be one of the following: 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 > Added more comments. OK, that makes sense. Thanks for the clarification! http://gerrit.cloudera.org:8080/#/c/18742/7/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: http://gerrit.cloudera.org:8080/#/c/18742/7/src/kudu/master/master_service.cc@105 PS7, Line 105: includes nit: including http://gerrit.cloudera.org:8080/#/c/18742/7/src/kudu/tablet/tablet-test.cc File src/kudu/tablet/tablet-test.cc: http://gerrit.cloudera.org:8080/#/c/18742/7/src/kudu/tablet/tablet-test.cc@942 PS7, Line 942: TYPED_TEST(TestImmutableColumn, TestUpsert) { Does it make sense to check for 'upsert_ignore_errors' metric values along with the 'upserts_as_updates' ones? http://gerrit.cloudera.org:8080/#/c/18742/7/src/kudu/tablet/tablet_metrics.cc File src/kudu/tablet/tablet_metrics.cc: http://gerrit.cloudera.org:8080/#/c/18742/7/src/kudu/tablet/tablet_metrics.cc@46 PS7, Line 46: Currently, the " : "only error is that the upsert is operated on a present row and " : "including a new cell value on an immutable column. Note that " : "the rows are still upsert but only ignore to update the " : "immutable cells How about: This metric counts the number of attempts to update a present row by changing the value of any of its immutable cells. Note that the rest of the cells (i.e. the mutable ones) in such case are updated accordingly to the operation's data. http://gerrit.cloudera.org:8080/#/c/18742/7/src/kudu/tserver/tserver.proto File src/kudu/tserver/tserver.proto: http://gerrit.cloudera.org:8080/#/c/18742/7/src/kudu/tserver/tserver.proto@431 PS7, Line 431: operation nit: operations -- 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: 7 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Wed, 10 Aug 2022 05:24:50 +0000 Gerrit-HasComments: Yes
