Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13059 )
Change subject: Bump Sentry version ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/13059/1/src/kudu/integration-tests/master_sentry-itest.cc File src/kudu/integration-tests/master_sentry-itest.cc: http://gerrit.cloudera.org:8080/#/c/13059/1/src/kudu/integration-tests/master_sentry-itest.cc@450 PS1, Line 450: TEST_F(MasterSentryTest, TestRenameTablePrivilegeTransfer) { It would be nice to add a short description for this test, summarizing the essence of what functionality is covered. http://gerrit.cloudera.org:8080/#/c/13059/1/src/kudu/integration-tests/master_sentry-itest.cc@472 PS1, Line 472: table_alterer.reset(client_->NewTableAlterer(Substitute("$0.$1", kDatabaseName, "b")) : ->RenameTo(Substitute("$0.$1", kDatabaseName, "c"))); : ASSERT_OK(table_alterer->Alter()); Not the part of this particular patch, is there any particular reason not to use RenameTable() instead of multiple lines of code here? Also, could you add a comment explaining the purpose of this extra renaming? Why it was not enough to have just the prior ALTER TABLE to complete successfully? http://gerrit.cloudera.org:8080/#/c/13059/1/src/kudu/sentry/mini_sentry.cc File src/kudu/sentry/mini_sentry.cc: http://gerrit.cloudera.org:8080/#/c/13059/1/src/kudu/sentry/mini_sentry.cc@219 PS1, Line 219: hive.sentry.server nit: could you also mention that this parameter is related to FLAG_server_name Kudu master's run-time flag? http://gerrit.cloudera.org:8080/#/c/13059/1/src/kudu/sentry/mini_sentry.cc@303 PS1, Line 303: server1 nit: Does it make sense to put some shared constant for this, so both this configuration file and the configuration file generated by mini_hms.c using the shared constant? http://gerrit.cloudera.org:8080/#/c/13059/1/src/kudu/sentry/sentry_policy_service.thrift File src/kudu/sentry/sentry_policy_service.thrift: http://gerrit.cloudera.org:8080/#/c/13059/1/src/kudu/sentry/sentry_policy_service.thrift@22 PS1, Line 22: 2c9a927a9e87cba0e4c0f34fc0b55887c6636927 It seems this should have been updated. -- To view, visit http://gerrit.cloudera.org:8080/13059 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I13f05345d2e6dcdadd74503d0d4524947b137d43 Gerrit-Change-Number: 13059 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 18 Apr 2019 17:42:11 +0000 Gerrit-HasComments: Yes
