[Impala-ASF-CR] IMPALA-9990: Support SET OWNER for Kudu tables
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16273 ) Change subject: IMPALA-9990: Support SET OWNER for Kudu tables .. IMPALA-9990: Support SET OWNER for Kudu tables KUDU-3090 adds the support for table ownership and exposes the API's of setting owner on creating and altering tables, which allows Impala to also pass to Kudu the new owner of the Kudu table for the ALTER TABLE SET OWNER statement. Specifically, based on the API of AlterTableOptions#setOwner(), this patch stores the ownership information of the Kudu table in the corresponding instance of AlterTableOptions, which will then be passed to Kudu via a KuduClient. Testing: - Added a FE test in AnalyzeKuduDDLTest.java to verify the statement could be correctly analyzed. - Added an E2E test in kudu_alter.test to verify the statement could be correctly executed when the integration between Kudu and HMS is not enabled. - Added an E2E test in kudu_hms_alter.test and verified that the statement could be correctly executed when the integration between Kudu and HMS is enabled after manually re-enabling TestKuduHMSIntegration::test_kudu_alter_table(). Note that this was not possible before IMPALA-10092 was resolved due to a bug in the class of CustomClusterTestSuite. In addition, we may need to delete the Kudu table 'simple' via a Kudu-Python client if the E2E test complains that the Kudu table already exists, which may be related to IMPALA-8751. - Manually verified that the views of Kudu server and HMS are consistent for a synchronized Kudu table after the ALTER TABLE SET OWNER statement even though the Kudu table was once an external and non-synchronized table, meaning that the owner from Kudu's perspective could be different than that from HMS' perspective. Such a discrepancy could be created if we execute the ALTER TABLE SET OWNER statement for an external Kudu table with the property of 'external.table.purge' being false. The test is performed manually because currently the Kudu-Python client adopted in Impala's E2E tests is not up to date so that the field of 'owner' cannot be accessed in the E2E tests. On the other hand, to verify the owner of a Kudu table from Kudu's perspective, we used the latest Kudu-Python client as provided at github.com/apache/kudu/tree/master/examples/python/basic-python-example. - Verified that the patch could pass the exhaustive tests in the DEBUG mode. Change-Id: I29d641efc8db314964bc5ee9828a86d4a44ae95c Reviewed-on: http://gerrit.cloudera.org:8080/16273 Reviewed-by: Vihang Karajgaonkar Tested-by: Impala Public Jenkins --- M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java M testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test M testdata/workloads/functional-query/queries/QueryTest/kudu_hms_alter.test 5 files changed, 65 insertions(+), 1 deletion(-) Approvals: Vihang Karajgaonkar: Looks good to me, approved Impala Public Jenkins: Verified -- 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: merged Gerrit-Change-Id: I29d641efc8db314964bc5ee9828a86d4a44ae95c Gerrit-Change-Number: 16273 Gerrit-PatchSet: 9 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-9990: Support SET OWNER for Kudu tables
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16273 ) Change subject: IMPALA-9990: Support SET OWNER for Kudu tables .. Patch Set 8: Verified+1 -- 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: 8 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Fri, 23 Oct 2020 04:01:24 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9990: Support SET OWNER for Kudu tables
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16273 ) Change subject: IMPALA-9990: Support SET OWNER for Kudu tables .. Patch Set 8: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/7536/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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: 8 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Thu, 22 Oct 2020 22:50:28 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9990: Support SET OWNER for Kudu tables
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16273 ) Change subject: IMPALA-9990: Support SET OWNER for Kudu tables .. Patch Set 8: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/6600/ DRY_RUN=false -- 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: 8 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Thu, 22 Oct 2020 22:30:34 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9990: Support SET OWNER for Kudu tables
Fang-Yu Rao has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/16273 ) Change subject: IMPALA-9990: Support SET OWNER for Kudu tables .. IMPALA-9990: Support SET OWNER for Kudu tables KUDU-3090 adds the support for table ownership and exposes the API's of setting owner on creating and altering tables, which allows Impala to also pass to Kudu the new owner of the Kudu table for the ALTER TABLE SET OWNER statement. Specifically, based on the API of AlterTableOptions#setOwner(), this patch stores the ownership information of the Kudu table in the corresponding instance of AlterTableOptions, which will then be passed to Kudu via a KuduClient. Testing: - Added a FE test in AnalyzeKuduDDLTest.java to verify the statement could be correctly analyzed. - Added an E2E test in kudu_alter.test to verify the statement could be correctly executed when the integration between Kudu and HMS is not enabled. - Added an E2E test in kudu_hms_alter.test and verified that the statement could be correctly executed when the integration between Kudu and HMS is enabled after manually re-enabling TestKuduHMSIntegration::test_kudu_alter_table(). Note that this was not possible before IMPALA-10092 was resolved due to a bug in the class of CustomClusterTestSuite. In addition, we may need to delete the Kudu table 'simple' via a Kudu-Python client if the E2E test complains that the Kudu table already exists, which may be related to IMPALA-8751. - Manually verified that the views of Kudu server and HMS are consistent for a synchronized Kudu table after the ALTER TABLE SET OWNER statement even though the Kudu table was once an external and non-synchronized table, meaning that the owner from Kudu's perspective could be different than that from HMS' perspective. Such a discrepancy could be created if we execute the ALTER TABLE SET OWNER statement for an external Kudu table with the property of 'external.table.purge' being false. The test is performed manually because currently the Kudu-Python client adopted in Impala's E2E tests is not up to date so that the field of 'owner' cannot be accessed in the E2E tests. On the other hand, to verify the owner of a Kudu table from Kudu's perspective, we used the latest Kudu-Python client as provided at github.com/apache/kudu/tree/master/examples/python/basic-python-example. - Verified that the patch could pass the exhaustive tests in the DEBUG mode. Change-Id: I29d641efc8db314964bc5ee9828a86d4a44ae95c --- M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java M testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test M testdata/workloads/functional-query/queries/QueryTest/kudu_hms_alter.test 5 files changed, 65 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/16273/8 -- 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: newpatchset Gerrit-Change-Id: I29d641efc8db314964bc5ee9828a86d4a44ae95c Gerrit-Change-Number: 16273 Gerrit-PatchSet: 8 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-9990: Support SET OWNER for Kudu tables
Andrew Wong 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:
(1 comment)
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) {
> Thanks Vihang for the feedback!
Thinking through the options, a few come to mind:
1. Return an error
2. Change the Kudu table's owner _and_ the HMS external table's owner
3. Just change the HMS external table's owner
So here's some food for thought: Is the ownership of a table meant to be
consistent across all references to that table (i.e. all external tables)? I
think the answer is no. Take the case in which we have multiple, separate
Impala clusters pointing at a Kudu table in our local cluster via external
tables. If one of those Impala clusters sets a different owner for its external
table, the metadata change wouldn't be reflected on all of the other clusters'
HMSs' external tables, since none of the HMS tables are synchronized. In that
regard, I think it would be weird if the ownership change of an external table
resulted in a change in ownership of the underlying Kudu table, since the
ownership change may not be reflected by all of its references (including the
external table that we just changed the ownership of!). Based on this, I would
lean away from option 2.
Digging into the other two options, I'm trying to better understand what
ownership is in the context of an external table. Is it meant to be used as the
owner of the underlying table? Or the owner of the HMS metadata entry (e.g. the
owner of a symlink)? Also, are external tables even able to have owners that
are not the owner of the underlying table? I would think the answer is yes, and
that "ownership" refers to ownership of the external table only, in which case
I would lean towards option 3, especially if that's allowed behavior today.
--
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
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Fang-Yu Rao
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vihang Karajgaonkar
Gerrit-Comment-Date: Thu, 22 Oct 2020 20:25:52 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9990: Support SET OWNER for Kudu tables
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
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Fang-Yu Rao
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vihang Karajgaonkar
Gerrit-Comment-Date: Thu, 22 Oct 2020 19:29:09 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9990: Support SET OWNER for Kudu tables
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
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Fang-Yu Rao
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vihang Karajgaonkar
Gerrit-Comment-Date: Wed, 21 Oct 2020 19:40:00 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9990: Support SET OWNER for Kudu tables
Impala Public Jenkins 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: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/7510/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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 Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Wed, 21 Oct 2020 19:34:14 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9990: Support SET OWNER for Kudu tables
Fang-Yu Rao has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/16273 ) Change subject: IMPALA-9990: Support SET OWNER for Kudu tables .. IMPALA-9990: Support SET OWNER for Kudu tables KUDU-3090 adds the support for table ownership and exposes the API's of setting owner on creating and altering tables, which allows Impala to also pass to Kudu the new owner of the Kudu table for the ALTER TABLE SET OWNER statement. Specifically, based on the API of AlterTableOptions#setOwner(), this patch stores the ownership information of the Kudu table in the corresponding instance of AlterTableOptions, which will then be passed to Kudu via a KuduClient. Testing: - Added a FE test in AnalyzeKuduDDLTest.java to verify the statement could be correctly analyzed. - Added an E2E test in kudu_alter.test to verify the statement could be correctly executed when the integration between Kudu and HMS is not enabled. - Added an E2E test in kudu_hms_alter.test and verified that the statement could be correctly executed when the integration between Kudu and HMS is enabled after manually re-enabling TestKuduHMSIntegration::test_kudu_alter_table(). Note that this was not possible before IMPALA-10092 was resolved due to a bug in the class of CustomClusterTestSuite. In addition, we may need to delete the Kudu table 'simple' via a Kudu-Python client if the E2E test complains that the Kudu table already exists, which may be related to IMPALA-8751. - Manually verified that the views of Kudu server and HMS are consistent for a synchronized Kudu table after the ALTER TABLE SET OWNER statement even though the Kudu table was once an external and non-synchronized table, meaning that the owner from Kudu's perspective could be different than that from HMS' perspective. Such a discrepancy could be created if we execute the ALTER TABLE SET OWNER statement for an external Kudu table with the property of 'external.table.purge' being false. The test is performed manually because currently the Kudu-Python client adopted in Impala's E2E tests is not up to date so that the field of 'owner' cannot be accessed in the E2E tests. On the other hand, to verify the owner of a Kudu table from Kudu's perspective, we used the latest Kudu-Python client as provided at github.com/apache/kudu/tree/master/examples/python/basic-python-example. - Verified that the patch could pass the exhaustive tests in the DEBUG mode. Change-Id: I29d641efc8db314964bc5ee9828a86d4a44ae95c --- M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java M testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test M testdata/workloads/functional-query/queries/QueryTest/kudu_hms_alter.test 5 files changed, 67 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/16273/6 -- 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: newpatchset Gerrit-Change-Id: I29d641efc8db314964bc5ee9828a86d4a44ae95c Gerrit-Change-Number: 16273 Gerrit-PatchSet: 6 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-9990: Support SET OWNER for Kudu tables
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16273 ) Change subject: IMPALA-9990: Support SET OWNER for Kudu tables .. Patch Set 4: Code-Review+1 (2 comments) http://gerrit.cloudera.org:8080/#/c/16273/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16273/2//COMMIT_MSG@29 PS2, Line 29: 92 was resolved due to a bug in th > Thanks Andrew! I have added that we manually tested that for a synchronized Great. Thanks for verifying! http://gerrit.cloudera.org:8080/#/c/16273/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16273/4//COMMIT_MSG@35 PS4, Line 35: Kutu nit: Kudu -- 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: 4 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Wed, 21 Oct 2020 18:02:01 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9990: Support SET OWNER for Kudu tables
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16273 ) Change subject: IMPALA-9990: Support SET OWNER for Kudu tables .. Patch Set 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/7498/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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: 4 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Tue, 20 Oct 2020 21:21:42 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9990: Support SET OWNER for Kudu tables
Fang-Yu Rao has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/16273 ) Change subject: IMPALA-9990: Support SET OWNER for Kudu tables .. IMPALA-9990: Support SET OWNER for Kudu tables KUDU-3090 adds the support for table ownership and exposes the API's of setting owner on creating and altering tables, which allows Impala to also pass to Kudu the new owner of the Kudu table for the ALTER TABLE SET OWNER statement. Specifically, based on the API of AlterTableOptions#setOwner(), this patch stores the ownership information of the Kudu table in the corresponding instance of AlterTableOptions, which will then be passed to Kudu via a KuduClient. Testing: - Added a FE test in AnalyzeKuduDDLTest.java to verify the statement could be correctly analyzed. - Added an E2E test in kudu_alter.test to verify the statement could be correctly executed when the integration between Kudu and HMS is not enabled. - Added an E2E test in kudu_hms_alter.test and verified that the statement could be correctly executed when the integration between Kudu and HMS is enabled after manually re-enabling TestKuduHMSIntegration::test_kudu_alter_table(). Note that this was not possible before IMPALA-10092 was resolved due to a bug in the class of CustomClusterTestSuite. In addition, we may need to delete the Kudu table 'simple' via a Kudu-Python client if the E2E test complains that the Kudu table already exists, which may be related to IMPALA-8751. - Manually verified that the views of Kudu server and HMS are consistent for a synchronized Kutu table after the ALTER TABLE SET OWNER statement even though the Kudu table was once an external and non-synchronized table, meaning that the owner from Kudu's perspective could be different than that from HMS' perspective. The test is performed manually because currently the Kudu-Python client adopted in Impala's E2E tests is not up to date so that the field of 'owner' cannot be accessed in the E2E tests. On the other hand, to verify the owner of a Kudu table from Kudu's perspective, we used the latest Kudu-Python client as provided at github.com/apache/kudu/tree/master/examples/python/basic-python-example. - Verified that the patch could pass the exhaustive tests in the DEBUG mode. Change-Id: I29d641efc8db314964bc5ee9828a86d4a44ae95c --- M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java M testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test M testdata/workloads/functional-query/queries/QueryTest/kudu_hms_alter.test 5 files changed, 67 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/16273/4 -- 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: newpatchset Gerrit-Change-Id: I29d641efc8db314964bc5ee9828a86d4a44ae95c Gerrit-Change-Number: 16273 Gerrit-PatchSet: 4 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-9990: Support SET OWNER for Kudu tables
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16273 ) Change subject: IMPALA-9990: Support SET OWNER for Kudu tables .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/16273/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16273/2//COMMIT_MSG@29 PS2, Line 29: Kudu-Python client if the E2E test If you already intend on testing using Kudu's Python bindings, it may also be worth testing the alter owner scenario and verifying that the Kudu's table.owner() matches that in the HMS. That'd be a good regression test for IMPALA-9990. http://gerrit.cloudera.org:8080/#/c/16273/2/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/2/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@550 PS2, Line 550: is changed to a managed table or an external : // table with the property of 'external.table.purge' being true from an external : // table with 'external.table.purge' not being true immediately before this method. nit: it's fine keeping this as is if it aligns more with Impala parlance, but this could be more succinct as, "if 'table' is changed to a synchronized Kudu table from an external, non-synchronized table before this method is called." -- 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: 2 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Mon, 19 Oct 2020 19:18:23 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9990: Support SET OWNER for Kudu tables
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16273 ) Change subject: IMPALA-9990: Support SET OWNER for Kudu tables .. Patch Set 2: Code-Review+1 -- 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: 2 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Mon, 19 Oct 2020 15:51:28 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9990: Support SET OWNER for Kudu tables
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16273 ) Change subject: IMPALA-9990: Support SET OWNER for Kudu tables .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/7474/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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: 2 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Sun, 18 Oct 2020 23:44:26 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9990: Support SET OWNER for Kudu tables
Fang-Yu Rao has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/16273 ) Change subject: IMPALA-9990: Support SET OWNER for Kudu tables .. IMPALA-9990: Support SET OWNER for Kudu tables KUDU-3090 adds the support for table ownership and exposes the API's of setting owner on creating and altering tables, which allows Impala to also pass to Kudu the new owner of the Kudu table for the ALTER TABLE SET OWNER statement. Specifically, based on the API of AlterTableOptions#setOwner(), this patch stores the ownership information of the Kudu table in the corresponding instance of AlterTableOptions, which will be passed to Kudu via a KuduClient. Testing: - Added a FE test in AnalyzeKuduDDLTest.java to verify the statement could be correctly analyzed. - Added an E2E test in kudu_alter.test to verify the statement could be correctly executed. - Added an E2E test in kudu_hms_alter.test and verified that the statement could be correctly executed after manually re-enabling TestKuduHMSIntegration::test_kudu_alter_table(). Note that this was not possible before IMPALA-10092 was resolved due to a bug in the class of CustomClusterTestSuite. In addition, we may need to delete the Kudu table 'simple' via a Kudu-Python client if the E2E test complains that the Kudu table already exists, which may be related to IMPALA-8751. - Verified that the patch could pass the exhaustive tests in the DEBUG mode. Change-Id: I29d641efc8db314964bc5ee9828a86d4a44ae95c --- M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java M testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test M testdata/workloads/functional-query/queries/QueryTest/kudu_hms_alter.test 5 files changed, 70 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/16273/2 -- 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: newpatchset Gerrit-Change-Id: I29d641efc8db314964bc5ee9828a86d4a44ae95c Gerrit-Change-Number: 16273 Gerrit-PatchSet: 2 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-9990: Support SET OWNER for Kudu tables
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16273 ) Change subject: IMPALA-9990: Support SET OWNER for Kudu tables .. Patch Set 1: Code-Review+1 (2 comments) http://gerrit.cloudera.org:8080/#/c/16273/1/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test File testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test: http://gerrit.cloudera.org:8080/#/c/16273/1/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test@51 PS1, Line 51: alter table simple set owner user non_owner > Does describe table list the owner? Thanks, Fang-Yu! http://gerrit.cloudera.org:8080/#/c/16273/1/testdata/workloads/functional-query/queries/QueryTest/kudu_hms_alter.test File testdata/workloads/functional-query/queries/QueryTest/kudu_hms_alter.test: http://gerrit.cloudera.org:8080/#/c/16273/1/testdata/workloads/functional-query/queries/QueryTest/kudu_hms_alter.test@53 PS1, Line 53: 'Owner has been altere.' > Thanks Attila! I see, thank you, Fang-Yu! -- 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: 1 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Wed, 02 Sep 2020 15:38:33 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9990: Support SET OWNER for Kudu tables
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 1: (3 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: Owner has been altered. May be a better message could be "Table has been updated." similar to the message in alterTable() method. 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: alterKuduTable(tbl, alterTableOptions, errMsg); > Thanks for digging into this! Based on my reading of the code, Impala checks for Kudu-HMS synchronization only for table creation/drops/renames. The alters are directly sent over to the Kudu with no interaction with HMS with the exception of alter_tbl properties which renames a table. It would be good to understand if owner field is also synchronized with HMS and if that is the case, we can do the following: 1. If the Kudu table is a synchronized table, we only do the Kudu operation and rely on Kudu to make the necessary HMS changes. 2. If the Kudu table is not a synchronized table, we will have to do a HMS alter operation and then the kudu operation. We should also make sure that the owner field of the describe table output is same as what the user sets for consistency. It would probably be easier to implement this if Kudu synchronizes the owner to HMS. http://gerrit.cloudera.org:8080/#/c/16273/1/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test File testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test: http://gerrit.cloudera.org:8080/#/c/16273/1/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test@51 PS1, Line 51: alter table simple set owner user non_owner > Thanks Attila! Does describe table list the owner? -- 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: 1 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Tue, 25 Aug 2020 20:12:35 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9990: Support SET OWNER for Kudu tables
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16273 ) Change subject: IMPALA-9990: Support SET OWNER for Kudu tables .. Patch Set 1: (1 comment) 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: alterKuduTable(tbl, alterTableOptions, errMsg); > Thanks Andrew! Thanks for digging into this! It's worth noting that at https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java#L680, the early return is only if the alter is a schema change in Kudu, per https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java#L872. Renames, for instance, are handled separately based on the HMS integration. We should probably follow suit with what we do for table renames, e.g. https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java#L3133. That approach takes into account the externality of a table, as well as Kudu's HMS integration. -- 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: 1 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Mon, 24 Aug 2020 19:16:39 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9990: Support SET OWNER for Kudu tables
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 1: (3 comments) Hi Andrew and Attila, I have replied your comments provided in the previous iteration and think it may be good to have a discussion about how Impala should implement SET OWNER for a Kudu table under different scenarios (Kudu-HMS integration enabled v.s. disabled). I also added Vihang as an additional reviewer since Vihang also worked on the functionality of creating Kudu tables in Impala and thus may be able to offer some insight into it. 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: alterKuduTable(tbl, alterTableOptions, errMsg); > This may be more a question for Kudu, but in what versions of Kudu is nativ Thanks Andrew! I do not have concrete answers to both of your questions, i.e., 1) what the behavior should be when HMS synchronization is disabled v.s. enabled, and 2) what the behavior should be when the Kudu table is external v.s. internal from Impala's perspective. But I did carry out an investigation of how a DDL statement of a Kudu table is implemented in Impala. According to CatalogOpExecutor#alterTable(), if this DDL statement is for a Kudu table, it looks like Impala would not contact HMS to apply the metadata change, which can be seen at https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java#L680. To be specific, after finishing CatalogOpExecutor#alterKuduTable(), CatalogOpExecutor#alterTable() returns immediately without going into the following switch block (https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java#L682-L845). Recall that for a DDL statement of a Kudu table, CatalogOpExecutor#alterKuduTable() would call the corresponding method in KuduCatalogOpExecutor.java (https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java#L682-L845), which in turn would call KuduCatalogOpExecutor#alterKuduTable() at https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java#L550-L561. Therefore, it is possible that the owner of a Kudu table from Kudu's perspective would be different than the owner from HMS's perspective if Kudu does not update HMS accordingly after the DDL statement. To verify this, I tried adding the following two statements before the return statement at https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java#L680 and found that the owner of a Kudu table could indeed be different if Kudu does not update the HMS on the ownership change. org.apache.hadoop.hive.metastore.api.Table msTbl = tbl.getMetaStoreTable().deepCopy(); String oldOwner = msTbl.getOwner(); To address this issue, we need a way to determine whether Kudu and HMS are synchronized. I found there is a method CatalogOpExecutor#isKuduHmsIntegrationEnabled() in CatalogOpExecutor.java at https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java#L1755-L1764, which could/may be used to determine whether the ownership information is synchronized between Kudu and HMS. Hence, based on this method, we could decide whether or not to update HMS on the ownership change of the Kudu table when Kudu and HMS is not synchronized. On the other hand, I have checked that using the current patch, alterSetOwner() could effectively alter the owner of the specified Kudu table no matter the Kudu table is external or internal. This could be manually verified by a revised Python program provided at https://github.com/apache/kudu/blob/master/examples/python/basic-python-example/basic_example.py. http://gerrit.cloudera.org:8080/#/c/16273/1/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test File testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test: http://gerrit.cloudera.org:8080/#/c/16273/1/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test@51 PS1, Line 51: alter table simple set owner user non_owner > is it possible to check if the owner of "simple" is actually "non_owner"? I Thanks Attila! It looks currently there is no command in Impala that allows a user to check the owner of a given table. A possible way to verify the owner of a Kudu table is through the Kudu-Python client currently adopted in Impala's end-to-end tests. For example, kudu.client at
[Impala-ASF-CR] IMPALA-9990: Support SET OWNER for Kudu tables
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16273 ) Change subject: IMPALA-9990: Support SET OWNER for Kudu tables .. Patch Set 1: (1 comment) 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: alterKuduTable(tbl, alterTableOptions, errMsg); This may be more a question for Kudu, but in what versions of Kudu is native ownership not supported? In those versions, what does this do? From the commit message here https://github.com/apache/kudu/commit/f0446b73630d75f6bf9c11b3fcce8953c557b578 it looks like the owner is synchronized between Kudu and the HMS. Attila, can you chime in with what should the behavior here be when the HMS synchronization is disabled vs enabled? Also, should there any difference in behavior when the Impala table is external/internal? -- 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: 1 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 03 Aug 2020 20:27:21 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9990: Support SET OWNER for Kudu tables
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16273 ) Change subject: IMPALA-9990: Support SET OWNER for Kudu tables .. Patch Set 1: (2 comments) > Patch Set 1: > > Hi all, please review my patch for supporting SET OWNER for Kudu tables in > Impala. > > This patch does not consider the cases in which a user is creating a new Kudu > table from scratch or creating a new Kudu table based on another table > (CTAS), since according to my investigation at IMPALA-9990, Impala indeed > passes the name of the logged in user instead of "impala" to Kudu when a Kudu > table is created. > > Let me know if you have any comment or suggestion. Thanks! http://gerrit.cloudera.org:8080/#/c/16273/1/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test File testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test: http://gerrit.cloudera.org:8080/#/c/16273/1/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test@51 PS1, Line 51: alter table simple set owner user non_owner is it possible to check if the owner of "simple" is actually "non_owner"? If yes, could you also add such tests for "create table"? http://gerrit.cloudera.org:8080/#/c/16273/1/testdata/workloads/functional-query/queries/QueryTest/kudu_hms_alter.test File testdata/workloads/functional-query/queries/QueryTest/kudu_hms_alter.test: http://gerrit.cloudera.org:8080/#/c/16273/1/testdata/workloads/functional-query/queries/QueryTest/kudu_hms_alter.test@53 PS1, Line 53: 'Owner has been altere.' nit: typo in altered. Is it normal that this test didn't fail? -- 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: 1 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 03 Aug 2020 09:42:21 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9990: Support SET OWNER for Kudu tables
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16273 ) Change subject: IMPALA-9990: Support SET OWNER for Kudu tables .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/6767/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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: 1 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 03 Aug 2020 05:58:36 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9990: Support SET OWNER for Kudu tables
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16273 ) Change subject: IMPALA-9990: Support SET OWNER for Kudu tables .. Patch Set 1: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/6766/ : Initial code review checks failed. See linked job for details on the failure. -- 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: 1 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Sun, 02 Aug 2020 23:58:54 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9990: Support SET OWNER for Kudu tables
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 1: > Patch Set 1: > > Build Failed > > https://jenkins.impala.io/job/gerrit-code-review-checks/6765/ : Initial code > review checks failed. See linked job for details on the failure. The build job failed because AlterTableOptions#setOwner() cannot be resolved (https://jenkins.impala.io/job/gerrit-code-review-checks/6765/artifact/https%3A%5E%5Ejenkins.impala.io%5Ejob%5Eubuntu-16.04-build-only%5E11967%5E.log). It is a bit strange since I thought after the patch for IMPALA-9903 (https://github.com/apache/impala/commit/da2999afd9ddc45d35141649d17db507e03ee9bf) has been merged, we will have a newer version of Kudu that contains the API of AlterTableOptions#setOwner(). In what follows I also provide the related error message. 23:26:41 [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.3:compile (default-compile) on project impala-frontend: Compilation failure 23:26:41 [ERROR] /home/ubuntu/tmp.7Xt03iIWrh/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java:[548,22] cannot find symbol 23:26:41 [ERROR] symbol: method setOwner(java.lang.String) 23:26:41 [ERROR] location: variable alterTableOptions of type org.apache.kudu.client.AlterTableOptions -- 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: 1 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Sun, 02 Aug 2020 23:35:03 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9990: Support SET OWNER for Kudu tables
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16273 ) Change subject: IMPALA-9990: Support SET OWNER for Kudu tables .. Patch Set 1: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/6765/ : Initial code review checks failed. See linked job for details on the failure. -- 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: 1 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Sun, 02 Aug 2020 23:26:54 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9990: Support SET OWNER for Kudu tables
Fang-Yu Rao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16273 Change subject: IMPALA-9990: Support SET OWNER for Kudu tables .. IMPALA-9990: Support SET OWNER for Kudu tables KUDU-3090 adds the support for table ownership and exposes the API's of setting owner on creating and altering tables, which allows Impala to also pass to Kudu the new owner of the Kudu table for the ALTER TABLE SET OWNER statement. Specifically, based on the API of AlterTableOptions#setOwner(), this patch stores the ownership information of the Kudu table in the corresponding instance of AlterTableOptions, which will be passed to Kudu via a KuduClient. Testing: - Added an FE test in AnalyzeKuduDDLTest.java to verify the statement could be correctly analyzed. - Added an E2E test in kudu_alter.test to verify the statement could be correctly executed. - Verified that the patch could pass the exhaustive tests in the DEBUG mode. Change-Id: I29d641efc8db314964bc5ee9828a86d4a44ae95c --- M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java M testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test M testdata/workloads/functional-query/queries/QueryTest/kudu_hms_alter.test 5 files changed, 34 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/16273/1 -- 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: newchange Gerrit-Change-Id: I29d641efc8db314964bc5ee9828a86d4a44ae95c Gerrit-Change-Number: 16273 Gerrit-PatchSet: 1 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Tim Armstrong
