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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Fri, 07 Sep 2018 17:12:29 +0000 Gerrit-HasComments: Yes
