Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18993 )
Change subject: KUDU-3353 [schema] Add an immutable attribute on column schema (part 3) ...................................................................... Patch Set 7: (8 comments) http://gerrit.cloudera.org:8080/#/c/18993/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18993/6//COMMIT_MSG@10 PS6, Line 10: includes > nit: contains Done http://gerrit.cloudera.org:8080/#/c/18993/6//COMMIT_MSG@28 PS6, Line 28: use > used Done http://gerrit.cloudera.org:8080/#/c/18993/6//COMMIT_MSG@29 PS6, Line 29: to ignore updating on immutable columns errors. > to ignore errors on updating cells of immutable columns Done http://gerrit.cloudera.org:8080/#/c/18993/7/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java File java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java: http://gerrit.cloudera.org:8080/#/c/18993/7/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java@558 PS7, Line 558: (step.getType() == AlterTableRequestPB.StepType.ALTER_COLUMN && : step.getAlterColumn().getDelta().getImmutable())) { > Just to double-check that I understand what's going on here. (a) Yes, it is true when removing the immutable attribute, as shown in L493, no matter 'immutable' is true or false, this condition evaluate to true. I've added a new test to check it in the latest patch set. (b) It is necessary. Consider a rare case that a Kudu cluster downgrade from a newer version to a lower version which dosen't support this feature. http://gerrit.cloudera.org:8080/#/c/18993/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/18993/7/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2787 PS7, Line 2787: /** : * Sends a request to the master to check if the cluster supports UpsertIgnore operations. : * @return true if the cluster supports upsert ignore operations : */ : @InterfaceAudience.Private : public Deferred<Boolean> supportsUpsertIgnoreOperations() { : PingRequest ping = PingRequest.makeMasterPingRequest( : this.masterTable, timer, defaultAdminOperationTimeoutMs); : ping.addRequiredFeature(Master.MasterFeatures.UPSERT_IGNORE_VALUE); : Deferred<PingResponse> response = sendRpcToTablet(ping); : return AsyncUtil.addBoth(response, new PingSupportsFeatureCallback()); : } > The supportsIgnoreOperations() above has been added since it was required f Thanks for you information. We've ever discussed about it here(in the file header): https://gerrit.cloudera.org/c/18835/4/src/kudu/client/client-test.cc, It's true that it's a kind of overkill, if a client could create such a table with immutable columns, of course the client can use UPSERT_IGNORE ops to operate on the cluster. I've also check what will the client get if server doesn't support UPSERT_IGNORE, it will get Status::Corruption error, see: https://github.com/apache/kudu/blob/master/src/kudu/common/row_operations.cc#L234, I think it's acceptable, I've removed these method in the latest patch set. http://gerrit.cloudera.org:8080/#/c/18993/7/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java: http://gerrit.cloudera.org:8080/#/c/18993/7/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java@500 PS7, Line 500: /** : * Sends a request to the master to check if the cluster supports upsert ignore operations. : * @return true if the cluster supports ignore operations : */ : @InterfaceAudience.Private : public boolean supportsUpsertIgnoreOperations() throws KuduException { : return joinAndHandleException(asyncClient.supportsUpsertIgnoreOperations()); : } > The supportsIgnoreOperations() above has been added since it was required f OK, removed. http://gerrit.cloudera.org:8080/#/c/18993/7/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java: http://gerrit.cloudera.org:8080/#/c/18993/7/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java@194 PS7, Line 194: An upsert ignore will : * ignore updating on immutable columns errors. This is useful when the upsert is operated : * on a row with immutable columns. > nit: how about Done http://gerrit.cloudera.org:8080/#/c/18993/7/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java: http://gerrit.cloudera.org:8080/#/c/18993/7/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java@2540 PS7, Line 2540: add a column with immutable attribute > to change the immutable attribute on a column Done -- To view, visit http://gerrit.cloudera.org:8080/18993 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifdfdcd123296803a3b5e856ec5eaac49c05b7f8d Gerrit-Change-Number: 18993 Gerrit-PatchSet: 7 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[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: Wed, 19 Oct 2022 13:32:36 +0000 Gerrit-HasComments: Yes
