Fang-Yu Rao 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: (2 comments) Hi all, I have replied to Vihang's comments on my patch. But I also raised some follow-up questions and thus we might need some feedback from Andrew and Attila on the Kudu project as for how to proceed. Thank you very much for the help! 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-ca Thanks Vihang for the feedback! I do not have a concrete answer to the question that in what scenario a user only wants to update HMS on the owner change. If we are sure that for an external and non-synchronized Kudu table (i.e., 'isSynchronizedKuduTable' is false), there is some sort of mechanism that could update the Kudu server on the owner change, then it would be reasonable to just update HMS on the owner change to save one RPC to the Kudu server from Impala. For example, the user issuing the ALTER TABLE SET OWNER statement always manually updates the Kudu server on the owner change, or the Kudu server is able to retrieve from the HMS the change to the owner of the Kudu table. Probably Andrew or Attila could offer some insight into it. As for throwing an error in the analysis phase, it could be done by adding a statement of "table_ = tableRef.getTable()" after https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java#L66 so that we are able to retrieve its corresponding org.apache.hadoop.hive.metastore.api.Table to determine whether the query is an attempt to change the owner of an external Kudu table. But I think a bigger question here is that do we really need to take into consideration the externality of a Kudu table. Can't we just simple update the Kudu server on the owner change unconditionally for a user executing a ALTER TABLE SET OWNER statement against a Kudu table? We probably need some suggestions from Andrew or Attila before I make any change to this patch set. Thanks! 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 Thanks Vihang for pointing this out! I will remove this since it looks like we don't need this check here. I actually think the previous check is not required either since in KuduTable.isSynchronizedTable(msTbl) above (https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/catalog/KuduTable.java#L142), the same check has been done already. -- 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: Thu, 22 Oct 2020 19:29:09 +0000 Gerrit-HasComments: Yes
