Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16273 )
Change subject: IMPALA-9990: Support SET OWNER for Kudu tables ...................................................................... Patch Set 6: (4 comments) http://gerrit.cloudera.org:8080/#/c/16273/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/16273/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@921 PS1, Line 921: TABLE operation for Ku > Thanks Vihang! Due to the fact that in the revised patch, we tackle the cas Thanks, that sounds good to me. http://gerrit.cloudera.org:8080/#/c/16273/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/16273/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3779 PS6, Line 3779: if (isSynchronizedKuduTable) { What does it mean to change owner of external kudu table? Is there a use-case where a user would want to do that? It seems counter intuitive to just change the owner in HMS. Do you think we should throw an error in analysis phase to not allow the user to alter owner of a external kudu table? See analyzeKuduTable() for example. http://gerrit.cloudera.org:8080/#/c/16273/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3781 PS6, Line 3781: Preconditions.checkState(tbl instanceof KuduTable); Not sure if this is really needed since the boolean is set above when this condition is true. http://gerrit.cloudera.org:8080/#/c/16273/1/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/16273/1/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@551 PS1, Line 551: // external, non-synchronized table before this > Thanks Andrew and Vihang for the pointers! May be I should have been clearer in my comment above. I meant that the KuduHMSIntegration is only checked with we do create/drop/rename since in those cases, we rely on Kudu to do the HMS changes as well. The whole concept of synchronized table is bit confusing because HMS introduced this new state or "external but will purge if removed" other than managed, external. We kind of combined that purgable external table into managed table since the behavior from the end user perspective is the same. We only check for this in create/drop/rename since it only affects the behavior of whether the table needs to be dropped in Kudu or not. With your patch, we expand this to whether the owner is updated in Kudu or not as well. -- To view, visit http://gerrit.cloudera.org:8080/16273 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I29d641efc8db314964bc5ee9828a86d4a44ae95c Gerrit-Change-Number: 16273 Gerrit-PatchSet: 6 Gerrit-Owner: Fang-Yu Rao <fangyu....@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Fang-Yu Rao <fangyu....@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Wed, 21 Oct 2020 19:40:00 +0000 Gerrit-HasComments: Yes
