You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2018/09/06 23:29:51 UTC
[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 <da...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 11 Sep 2018 17:46:19 +0000
Gerrit-HasComments: No
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 11 Sep 2018 18:07:43 +0000
Gerrit-HasComments: No
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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<const string&> owner,
> warning: the parameter 'owner' is copied for each invocation but only used
This is a poor suggestion, optional<const string&> 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, &client_);
> 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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 07 Sep 2018 17:12:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 07 Sep 2018 01:43:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 10 Sep 2018 21:52:34 +0000
Gerrit-HasComments: Yes
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 10 Sep 2018 21:57:05 +0000
Gerrit-HasComments: No
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 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@20
PS1, Line 20: hte
the
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?
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.
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.
http://gerrit.cloudera.org:8080/#/c/11398/1/src/kudu/integration-tests/master_hms-itest.cc@637
PS1, Line 637: cluster_->CreateClient(nullptr, &client_);
ASSERT_OK
--
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 06 Sep 2018 23:58:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 10 Sep 2018 20:07:06 +0000
Gerrit-HasComments: Yes
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 06 Sep 2018 23:58:39 +0000
Gerrit-HasComments: Yes
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Sat, 08 Sep 2018 01:05:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
---
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 07 Sep 2018 21:35:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 11 Sep 2018 18:10:57 +0000
Gerrit-HasComments: No
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 10 Sep 2018 19:54:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 11 Sep 2018 22:45:24 +0000
Gerrit-HasComments: No
[kudu-CR] HMS integration: set table owner field in HMS table metadata
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 10 Sep 2018 21:52:15 +0000
Gerrit-HasComments: No