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 8: (19 comments) Thank you for your patient review. 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: ttempt to mod > I meant whether it's possible to tell between two different error condition Done 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 new column attribute IMMUTABLE, meaning the colu > Add a new column attribute IMMUTABLE Done http://gerrit.cloudera.org:8080/#/c/18742/7//COMMIT_MSG@10 PS7, Line 10: cell > meaning Done http://gerrit.cloudera.org:8080/#/c/18742/7//COMMIT_MSG@13 PS7, Line 13: An attempt to modify an immutable cell of an existing row by : UPDATE or UPSERT operation results in returning the newly added : Status::IsImmutab > How about: Done http://gerrit.cloudera.org:8080/#/c/18742/7//COMMIT_MSG@16 PS7, Line 16: UPDATE_IGNORE > Thanks for clarifying! Could you add that into the commit description as w Done http://gerrit.cloudera.org:8080/#/c/18742/7//COMMIT_MSG@17 PS7, Line 17: update errors > update errors Done http://gerrit.cloudera.org:8080/#/c/18742/7//COMMIT_MSG@17 PS7, Line 17: Note that : the rest of the columns are updated accordingly to the : operation's data, o > Note that the rest of the columns are updated accordingly to the operation' Done http://gerrit.cloudera.org:8080/#/c/18742/7//COMMIT_MSG@22 PS7, Line 22: : Some use cases: : 1. A column represents a semantically constant entity. The : corre > How about: Done http://gerrit.cloudera.org:8080/#/c/18742/7//COMMIT_MSG@26 PS7, Line 26: 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 > How about: Done http://gerrit.cloudera.org:8080/#/c/18742/7//COMMIT_MSG@30 PS7, Line 30: e correspondin > on the server side Done http://gerrit.cloudera.org:8080/#/c/18742/7//COMMIT_MSG@30 PS7, Line 30: , : is the same wi > necessary changes on the client side Done 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? I plan to implement it in a separate patch. 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? I plan to implement it in a separate patch because it's closer to java client side. 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? I plan to implement it in a separate patch because it's closer to cpp client side. 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: As of now, the error cou > How about: Done 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: includin > nit: including Done 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 Good idea! Done 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: 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 operatio > How about: Done 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 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: 8 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 16:36:00 +0000 Gerrit-HasComments: Yes
