Alexey Serbin 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) I apologize for the late response. Overall looks good, just a few questions about methods that look like an extra and to double-check my understanding. 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 This is to avoid the '... includes ..., including ...' pattern http://gerrit.cloudera.org:8080/#/c/18993/6//COMMIT_MSG@28 PS6, Line 28: use used 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 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. What if trying to alter a column, removing the immutable attribute from it? In that case: (a) Does this condition evaluate to true? (b) Is it necessary to request the support for the newly added feature in that case? Thanks! 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 for Kudu backup&restore. What's the anticipated use case for these? If that's just some theoretical one, I'd suggest not adding this method yet according to the YAGNI principle: https://www.martinfowler.com/bliki/Yagni.html 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 for Kudu backup&restore. What's the anticipated use case for these? If they are not used anywhere (but the tests added), maybe we don't really need to have this method, even if it's a part of the priva API? If you have some particular use-case to be added with next patch or so, maybe add these methods along with that new functionality? 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 The operation ignores errors of updating immutable cells in a row. This is useful when upserting rows in a table with immutable columns. 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 -- 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: Tue, 18 Oct 2022 23:51:04 +0000 Gerrit-HasComments: Yes
