[kudu-CR] HMS integration: set table owner field in HMS table metadata
Dan Burkert has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11398 ) Change subject: HMS integration: set table owner field in HMS table metadata .. HMS integration: set table owner field in HMS table metadata Other systems use table ownership for purposes like assigning privileges. This patch sets the owner field in the HMS for Kudu tables to the client's user name. A follow-up patch will add additional APIs to the client CreateTable builders which will allow clients to override the owner, for situations in which the client is actually proxying through the table creation on behalf of a different user. The HMS fix tool will create HMS table metadata for Kudu tables which are missing it. This replacement table metadata will omit the table owner, since it can't be reconstructed just from the Kudu table metadata. This is the conservative approach, if we want to extend the tool to allow passing in the owner as a flag we can do that in the future. Change-Id: I1a25aa0bb52bdd28df28a078fe91f55db9e29482 Reviewed-on: http://gerrit.cloudera.org:8080/11398 Tested-by: Kudu Jenkins Reviewed-by: Dan Burkert --- M src/kudu/hms/hms_catalog-test.cc M src/kudu/hms/hms_catalog.cc M src/kudu/hms/hms_catalog.h M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_hms.cc 7 files changed, 145 insertions(+), 55 deletions(-) Approvals: Kudu Jenkins: Verified Dan Burkert: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/11398 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I1a25aa0bb52bdd28df28a078fe91f55db9e29482 Gerrit-Change-Number: 11398 Gerrit-PatchSet: 8 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11398 ) Change subject: HMS integration: set table owner field in HMS table metadata .. Patch Set 7: Code-Review+2 Carrying through Hao's +2 and Adar's +1. -- To view, visit http://gerrit.cloudera.org:8080/11398 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1a25aa0bb52bdd28df28a078fe91f55db9e29482 Gerrit-Change-Number: 11398 Gerrit-PatchSet: 7 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 11 Sep 2018 22:45:24 + Gerrit-HasComments: No
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11398 ) Change subject: HMS integration: set table owner field in HMS table metadata .. Patch Set 6: The LINT failure is related to the other patch. https://gerrit.cloudera.org/#/c/11415/ -- To view, visit http://gerrit.cloudera.org:8080/11398 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1a25aa0bb52bdd28df28a078fe91f55db9e29482 Gerrit-Change-Number: 11398 Gerrit-PatchSet: 6 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 11 Sep 2018 18:10:57 + Gerrit-HasComments: No
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11398 ) Change subject: HMS integration: set table owner field in HMS table metadata .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11398 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1a25aa0bb52bdd28df28a078fe91f55db9e29482 Gerrit-Change-Number: 11398 Gerrit-PatchSet: 6 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 11 Sep 2018 18:07:43 + Gerrit-HasComments: No
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11398 ) Change subject: HMS integration: set table owner field in HMS table metadata .. Patch Set 6: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/11398 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1a25aa0bb52bdd28df28a078fe91f55db9e29482 Gerrit-Change-Number: 11398 Gerrit-PatchSet: 6 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 11 Sep 2018 17:46:19 + Gerrit-HasComments: No
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11398 to look at the new patch set (#5). Change subject: HMS integration: set table owner field in HMS table metadata .. HMS integration: set table owner field in HMS table metadata Other systems use table ownership for purposes like assigning privileges. This patch sets the owner field in the HMS for Kudu tables to the client's user name. A follow-up patch will add additional APIs to the client CreateTable builders which will allow clients to override the owner, for situations in which the client is actually proxying through the table creation on behalf of a different user. The HMS fix tool will create HMS table metadata for Kudu tables which are missing it. This replacement table metadata will omit the table owner, since it can't be reconstructed just from the Kudu table metadata. This is the conservative approach, if we want to extend the tool to allow passing in the owner as a flag we can do that in the future. Change-Id: I1a25aa0bb52bdd28df28a078fe91f55db9e29482 --- M src/kudu/hms/hms_catalog-test.cc M src/kudu/hms/hms_catalog.cc M src/kudu/hms/hms_catalog.h M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_hms.cc 7 files changed, 145 insertions(+), 55 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/11398/5 -- To view, visit http://gerrit.cloudera.org:8080/11398 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1a25aa0bb52bdd28df28a078fe91f55db9e29482 Gerrit-Change-Number: 11398 Gerrit-PatchSet: 5 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11398 ) Change subject: HMS integration: set table owner field in HMS table metadata .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11398 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1a25aa0bb52bdd28df28a078fe91f55db9e29482 Gerrit-Change-Number: 11398 Gerrit-PatchSet: 4 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Mon, 10 Sep 2018 21:57:05 + Gerrit-HasComments: No
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11398 ) Change subject: HMS integration: set table owner field in HMS table metadata .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/11398/3/src/kudu/hms/hms_catalog.h File src/kudu/hms/hms_catalog.h: http://gerrit.cloudera.org:8080/#/c/11398/3/src/kudu/hms/hms_catalog.h@64 PS3, Line 64: omitted > Nit: just one 'm'. Done -- To view, visit http://gerrit.cloudera.org:8080/11398 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1a25aa0bb52bdd28df28a078fe91f55db9e29482 Gerrit-Change-Number: 11398 Gerrit-PatchSet: 4 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Mon, 10 Sep 2018 21:52:34 + Gerrit-HasComments: Yes
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11398 ) Change subject: HMS integration: set table owner field in HMS table metadata .. Patch Set 4: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/11398 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1a25aa0bb52bdd28df28a078fe91f55db9e29482 Gerrit-Change-Number: 11398 Gerrit-PatchSet: 4 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Mon, 10 Sep 2018 21:52:15 + Gerrit-HasComments: No
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11398 to look at the new patch set (#4). Change subject: HMS integration: set table owner field in HMS table metadata .. HMS integration: set table owner field in HMS table metadata Other systems use table ownership for purposes like assigning privileges. This patch sets the owner field in the HMS for Kudu tables to the client's user name. A follow-up patch will add additional APIs to the client CreateTable builders which will allow clients to override the owner, for situations in which the client is actually proxying through the table creation on behalf of a different user. The HMS fix tool will create HMS table metadata for Kudu tables which are missing it. This replacement table metadata will omit the table owner, since it can't be reconstructed just from the Kudu table metadata. This is the conservative approach, if we want to extend the tool to allow passing in the owner as a flag we can do that in the future. Change-Id: I1a25aa0bb52bdd28df28a078fe91f55db9e29482 --- M src/kudu/hms/hms_catalog-test.cc M src/kudu/hms/hms_catalog.cc M src/kudu/hms/hms_catalog.h M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_hms.cc 7 files changed, 145 insertions(+), 55 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/11398/4 -- To view, visit http://gerrit.cloudera.org:8080/11398 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1a25aa0bb52bdd28df28a078fe91f55db9e29482 Gerrit-Change-Number: 11398 Gerrit-PatchSet: 4 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11398 ) Change subject: HMS integration: set table owner field in HMS table metadata .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/11398/3/src/kudu/hms/hms_catalog.h File src/kudu/hms/hms_catalog.h: http://gerrit.cloudera.org:8080/#/c/11398/3/src/kudu/hms/hms_catalog.h@64 PS3, Line 64: ommitted Nit: just one 'm'. But I was hoping you'd elaborate on what it means for the table to lack an owner. -- To view, visit http://gerrit.cloudera.org:8080/11398 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1a25aa0bb52bdd28df28a078fe91f55db9e29482 Gerrit-Change-Number: 11398 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Mon, 10 Sep 2018 20:07:06 + Gerrit-HasComments: Yes
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11398 ) Change subject: HMS integration: set table owner field in HMS table metadata .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/11398/2/src/kudu/hms/hms_catalog.h File src/kudu/hms/hms_catalog.h: http://gerrit.cloudera.org:8080/#/c/11398/2/src/kudu/hms/hms_catalog.h@139 PS2, Line 139: // Sets the Kudu-specific fields in the table without overwriting unrelated fields. > Can you please add a comment here that owner information will not be set if Done http://gerrit.cloudera.org:8080/#/c/11398/2/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/11398/2/src/kudu/tools/kudu-tool-test.cc@2311 PS2, Line 2311: CreateLegacyHmsTable > Do you think it makes sense to test legacy HMS table with ownership as well Great point. Added a test case for a legacy table w/o owner, and additional checking that table ownership is preserved. -- To view, visit http://gerrit.cloudera.org:8080/11398 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1a25aa0bb52bdd28df28a078fe91f55db9e29482 Gerrit-Change-Number: 11398 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Mon, 10 Sep 2018 19:54:38 + Gerrit-HasComments: Yes
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11398 to look at the new patch set (#3). Change subject: HMS integration: set table owner field in HMS table metadata .. HMS integration: set table owner field in HMS table metadata Other systems use table ownership for purposes like assigning privileges. This patch sets the owner field in the HMS for Kudu tables to the client's user name. A follow-up patch will add additional APIs to the client CreateTable builders which will allow clients to override the owner, for situations in which the client is actually proxying through the table creation on behalf of a different user. The HMS fix tool will create HMS table metadata for Kudu tables which are missing it. This replacement table metadata will omit the table owner, since it can't be reconstructed just from the Kudu table metadata. This is the conservative approach, if we want to extend the tool to allow passing in the owner as a flag we can do that in the future. Change-Id: I1a25aa0bb52bdd28df28a078fe91f55db9e29482 --- M src/kudu/hms/hms_catalog-test.cc M src/kudu/hms/hms_catalog.cc M src/kudu/hms/hms_catalog.h M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_hms.cc 7 files changed, 143 insertions(+), 55 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/11398/3 -- To view, visit http://gerrit.cloudera.org:8080/11398 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1a25aa0bb52bdd28df28a078fe91f55db9e29482 Gerrit-Change-Number: 11398 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11398 ) Change subject: HMS integration: set table owner field in HMS table metadata .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11398/2/src/kudu/hms/hms_catalog.h File src/kudu/hms/hms_catalog.h: http://gerrit.cloudera.org:8080/#/c/11398/2/src/kudu/hms/hms_catalog.h@139 PS2, Line 139: // Sets the Kudu-specific fields in the table without overwriting unrelated fields. Can you please add a comment here that owner information will not be set if not provided? Thanks! -- To view, visit http://gerrit.cloudera.org:8080/11398 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1a25aa0bb52bdd28df28a078fe91f55db9e29482 Gerrit-Change-Number: 11398 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Sat, 08 Sep 2018 01:05:03 + Gerrit-HasComments: Yes
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11398 ) Change subject: HMS integration: set table owner field in HMS table metadata .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11398/2/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/11398/2/src/kudu/tools/kudu-tool-test.cc@2311 PS2, Line 2311: CreateLegacyHmsTable Do you think it makes sense to test legacy HMS table with ownership as well? In case the users were previously in a version of Impala with ownership enabled. -- To view, visit http://gerrit.cloudera.org:8080/11398 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1a25aa0bb52bdd28df28a078fe91f55db9e29482 Gerrit-Change-Number: 11398 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Fri, 07 Sep 2018 21:35:45 + Gerrit-HasComments: Yes
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Andrew Wong has removed Andrew Wong from this change. ( http://gerrit.cloudera.org:8080/11398 ) Change subject: HMS integration: set table owner field in HMS table metadata .. Removed reviewer Andrew Wong. -- To view, visit http://gerrit.cloudera.org:8080/11398 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I1a25aa0bb52bdd28df28a078fe91f55db9e29482 Gerrit-Change-Number: 11398 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Hello Tidy Bot, Andrew Wong, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11398 to look at the new patch set (#2). Change subject: HMS integration: set table owner field in HMS table metadata .. HMS integration: set table owner field in HMS table metadata Other systems use table ownership for purposes like assigning privileges. This patch sets the owner field in the HMS for Kudu tables to the client's user name. A follow-up patch will add additional APIs to the client CreateTable builders which will allow clients to override the owner, for situations in which the client is actually proxying through the table creation on behalf of a different user. The HMS fix tool will create HMS table metadata for Kudu tables which are missing it. This replacement table metadata will omit the table owner, since it can't be reconstructed just from the Kudu table metadata. This is the conservative approach, if we want to extend the tool to allow passing in the owner as a flag we can do that in the future. Change-Id: I1a25aa0bb52bdd28df28a078fe91f55db9e29482 --- M src/kudu/hms/hms_catalog-test.cc M src/kudu/hms/hms_catalog.cc M src/kudu/hms/hms_catalog.h M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_hms.cc 7 files changed, 100 insertions(+), 40 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/11398/2 -- To view, visit http://gerrit.cloudera.org:8080/11398 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1a25aa0bb52bdd28df28a078fe91f55db9e29482 Gerrit-Change-Number: 11398 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11398 ) Change subject: HMS integration: set table owner field in HMS table metadata .. Patch Set 1: (14 comments) http://gerrit.cloudera.org:8080/#/c/11398/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11398/1//COMMIT_MSG@11 PS1, Line 11: A follow-up patch > Do we need to mention 'ALTER TABLE SET OWNER' is not supported for Kudu tab I don't think it's worth mentioning since we aren't exposing any other way to modify HMS-only metadata (comments, table properties, etc.). http://gerrit.cloudera.org:8080/#/c/11398/1//COMMIT_MSG@16 PS1, Line 16: fix > The same for 'upgrade' tool? The upgrade tool has been combined with the fix tool, so yes. http://gerrit.cloudera.org:8080/#/c/11398/1//COMMIT_MSG@17 PS1, Line 17: This replacement table metadata will omit the table : owner > Did you consider passing in the table owner name as a flag to the tool? Is After thinking about this a bit I think the privilege escalation issues I was concerned about aren't an issue, since the admin who is running the tool must otherwise have privileges to create the table, at which point they could set the ownership themselves (through a different HMS client). As far as whether we want a flag, I'd prefer to hold off for now on providing that, it can always be added in the future. http://gerrit.cloudera.org:8080/#/c/11398/1//COMMIT_MSG@20 PS1, Line 20: hte > nit: the Done http://gerrit.cloudera.org:8080/#/c/11398/1//COMMIT_MSG@22 PS1, Line 22: omit : table ownership information > What will happen of ownership is omitted? Will that information be blank in Yes, the field will be empty in the HMS. http://gerrit.cloudera.org:8080/#/c/11398/1//COMMIT_MSG@22 PS1, Line 22: As such I think it's safer to omit : table ownership information in this case. An admin can re-assign the : table ownership through Beeline or Impala given sufficient credentials. > What does the resulting ownerless table metadata mean in terms of privilege It just means no user is given the set of owner privileges. To change the owner of the table Sentry requires some set of admin privileges. http://gerrit.cloudera.org:8080/#/c/11398/1/src/kudu/hms/hms_catalog-test.cc File src/kudu/hms/hms_catalog-test.cc: PS1: > Any tests where owner is omitted? Done http://gerrit.cloudera.org:8080/#/c/11398/1/src/kudu/hms/hms_catalog.h File src/kudu/hms/hms_catalog.h: http://gerrit.cloudera.org:8080/#/c/11398/1/src/kudu/hms/hms_catalog.h@62 PS1, Line 62: // Creates a new table entry in the HMS. > Should mention what happens if 'owner' is omitted. Done http://gerrit.cloudera.org:8080/#/c/11398/1/src/kudu/hms/hms_catalog.cc File src/kudu/hms/hms_catalog.cc: http://gerrit.cloudera.org:8080/#/c/11398/1/src/kudu/hms/hms_catalog.cc@487 PS1, Line 487: optional owner, > warning: the parameter 'owner' is copied for each invocation but only used This is a poor suggestion, optional should be trivially copyable. http://gerrit.cloudera.org:8080/#/c/11398/1/src/kudu/integration-tests/master_hms-itest.cc File src/kudu/integration-tests/master_hms-itest.cc: http://gerrit.cloudera.org:8080/#/c/11398/1/src/kudu/integration-tests/master_hms-itest.cc@74 PS1, Line 74: virtual bool EnableKerberos() { > Can be private. Done http://gerrit.cloudera.org:8080/#/c/11398/1/src/kudu/integration-tests/master_hms-itest.cc@637 PS1, Line 637: cluster_->CreateClient(nullptr, _); > ASSERT_OK Done http://gerrit.cloudera.org:8080/#/c/11398/1/src/kudu/integration-tests/master_hms-itest.cc@643 PS1, Line 643: table.owner, "test-user" > nit: expected value should go first Done http://gerrit.cloudera.org:8080/#/c/11398/1/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/11398/1/src/kudu/tools/kudu-tool-test.cc@2445 PS1, Line 2445: TestCheckAndAutomaticFixHmsMetadata > Does this include any test cases that the owner is omitted? No, since Kudu shouldn't create a table without an owner in any circumstances. http://gerrit.cloudera.org:8080/#/c/11398/1/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/11398/1/src/kudu/tools/tool_action_hms.cc@168 PS1, Line 168: boost::none > Does this mean if the HMS table have ownership information but Kudu table d The effect of this is that the table owner field is not checked. -- To view, visit http://gerrit.cloudera.org:8080/11398 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1a25aa0bb52bdd28df28a078fe91f55db9e29482 Gerrit-Change-Number: 11398 Gerrit-PatchSet: 1 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Andrew Wong
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11398 ) Change subject: HMS integration: set table owner field in HMS table metadata .. Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/11398/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11398/1//COMMIT_MSG@11 PS1, Line 11: A follow-up patch Do we need to mention 'ALTER TABLE SET OWNER' is not supported for Kudu tables? http://gerrit.cloudera.org:8080/#/c/11398/1//COMMIT_MSG@16 PS1, Line 16: fix The same for 'upgrade' tool? http://gerrit.cloudera.org:8080/#/c/11398/1//COMMIT_MSG@22 PS1, Line 22: omit : table ownership information What will happen of ownership is omitted? Will that information be blank in the HMS? I guess this is a similar question to Andrew's. http://gerrit.cloudera.org:8080/#/c/11398/1/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/11398/1/src/kudu/tools/kudu-tool-test.cc@2445 PS1, Line 2445: TestCheckAndAutomaticFixHmsMetadata Does this include any test cases that the owner is omitted? http://gerrit.cloudera.org:8080/#/c/11398/1/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/11398/1/src/kudu/tools/tool_action_hms.cc@168 PS1, Line 168: boost::none Does this mean if the HMS table have ownership information but Kudu table doesn't, the check will fail? -- To view, visit http://gerrit.cloudera.org:8080/11398 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1a25aa0bb52bdd28df28a078fe91f55db9e29482 Gerrit-Change-Number: 11398 Gerrit-PatchSet: 1 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Fri, 07 Sep 2018 01:43:22 + Gerrit-HasComments: Yes
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11398 ) Change subject: HMS integration: set table owner field in HMS table metadata .. Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/11398/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11398/1//COMMIT_MSG@17 PS1, Line 17: This replacement table metadata will omit the table : owner Did you consider passing in the table owner name as a flag to the tool? Is that possible given the admin's privileges? http://gerrit.cloudera.org:8080/#/c/11398/1//COMMIT_MSG@20 PS1, Line 20: hte nit: the http://gerrit.cloudera.org:8080/#/c/11398/1//COMMIT_MSG@22 PS1, Line 22: As such I think it's safer to omit : table ownership information in this case. An admin can re-assign the : table ownership through Beeline or Impala given sufficient credentials. What does the resulting ownerless table metadata mean in terms of privileges? What are the sufficient credentials to modifying an ownerless table? Is the behavior of these tables dependent on the privilege-enforcing entity (e.g. Sentry)? http://gerrit.cloudera.org:8080/#/c/11398/1/src/kudu/integration-tests/master_hms-itest.cc File src/kudu/integration-tests/master_hms-itest.cc: http://gerrit.cloudera.org:8080/#/c/11398/1/src/kudu/integration-tests/master_hms-itest.cc@643 PS1, Line 643: table.owner, "test-user" nit: expected value should go first -- To view, visit http://gerrit.cloudera.org:8080/11398 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1a25aa0bb52bdd28df28a078fe91f55db9e29482 Gerrit-Change-Number: 11398 Gerrit-PatchSet: 1 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Thu, 06 Sep 2018 23:58:39 + Gerrit-HasComments: Yes
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Hello Andrew Wong, Hao Hao, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/11398 to review the following change. Change subject: HMS integration: set table owner field in HMS table metadata .. HMS integration: set table owner field in HMS table metadata Other systems use table ownership for purposes like assigning privileges. This patch sets the owner field in the HMS for Kudu tables to the client's user name. A follow-up patch will add additional APIs to the client CreateTable builders which will allow clients to override the owner, for situations in which the client is actually proxying through the table creation on behalf of a different user. The HMS fix tool will create HMS table metadata for Kudu tables which are missing it. This replacement table metadata will omit the table owner, since it can't be reconstructed just from the Kudu table metadata. I considered defaulting to the logged-in user which is fixing hte metadata (the admin user), but this could result in privilege escalation if the admin user had rights to create/repair HMS tables, but not read/write access to the table. As such I think it's safer to omit table ownership information in this case. An admin can re-assign the table ownership through Beeline or Impala given sufficient credentials. Change-Id: I1a25aa0bb52bdd28df28a078fe91f55db9e29482 --- M src/kudu/hms/hms_catalog-test.cc M src/kudu/hms/hms_catalog.cc M src/kudu/hms/hms_catalog.h M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_hms.cc 7 files changed, 88 insertions(+), 36 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/11398/1 -- To view, visit http://gerrit.cloudera.org:8080/11398 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I1a25aa0bb52bdd28df28a078fe91f55db9e29482 Gerrit-Change-Number: 11398 Gerrit-PatchSet: 1 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao