Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11797 )
Change subject: [sentry] Integrate AuthzProvider into CatalogManager ...................................................................... Patch Set 7: (22 comments) http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/hms_itest-base.h File src/kudu/integration-tests/hms_itest-base.h: http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/hms_itest-base.h@52 PS7, Line 52: using client::KuduColumnSchema; : using client::KuduSchema; : using client::KuduSchemaBuilder; : using client::KuduTable; : using client::KuduTableCreator; : using client::sp::shared_ptr; : using hms::HmsClient; : using std::string; : using std::unique_ptr; : using strings::Substitute; I'm not sure that 'using ...' in a header file is a good idea. http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/hms_itest-base.h@68 PS7, Line 68: RETURN_NOT_OK(cluster_->hms()->Stop()); : return Status::OK(); nit for brevity here and below: maybe, just return the status of the last function called, i.e. instead of these two lines return cluster_->hms()->Stop(); http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/hms_itest-base.h@129 PS7, Line 129: return hms_client_->AlterTable(database_name, old_table_name, table); Just out of curiosity (not sure it's a matter of concern right here): What happens if the table is altered inbetween (e.g., new columns added)? Would those modifications be lost when AlterTable() below is called? http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/hms_itest-base.h@154 PS7, Line 154: KuduSchema schema = table->schema(); const auto& schema = table->schema(); and move closer to the point of the usage? http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/hms_itest-base.h@193 PS7, Line 193: env_ctx.__set_properties({ std::make_pair(hms::HmsClient::kKuduMasterEventKey, "true") }); Would the shorter option like env_ctx.__set_properties({ { HmsClient::kKuduMasterEventKey, "true" } }); work here (it's C++11, right)? http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/hms_itest-base.h@199 PS7, Line 199: nit: drop the extra line? http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_hms-itest.cc File src/kudu/integration-tests/master_hms-itest.cc: http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_hms-itest.cc@46 PS7, Line 46: using boost::none; : using client::KuduTable; : using client::KuduTableAlterer; : using client::sp::shared_ptr; : using cluster::ExternalMiniClusterOptions; : using hms::HmsClient; : using std::string; : using std::unique_ptr; : using std::vector; : using strings::Substitute; nit: could you move these 'using' out of the 'namespace ...' scope? http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry-itest.cc File src/kudu/integration-tests/master_sentry-itest.cc: http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry-itest.cc@35 PS7, Line 35: using std::set; : using std::string; : using std::vector; : using strings::Substitute; nit: could you move this out of the 'namespace ...' scope? http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry-itest.cc@51 PS7, Line 51: ASSERT_EQ(0, tables.size()); nit: ASSERT_TRUE(tables.empty()); http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry-itest.cc@62 PS7, Line 62: std:: drop the prefix (there is corresponding 'using ...' above) http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry-itest.cc@62 PS7, Line 62: std:: ditto http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry-itest.cc@75 PS7, Line 75: Substitute("$0.$1", desc.database, desc.table), : Substitute("$0.$1", desc.database, desc.new_table)); here and in other places in this file: maybe, introduce a couple of const variables and use the in the test instead of calling Substitute() multiple times for the same parameters? E.g.: const auto table_name = Substitute("$0.$1", desc.database, desc.table); const auto new_table_name = Substitute("$0.$1", desc.database, desc.new_table); ... Status s = desc.funcs.op_func(this, table_name, new_table_name); ... http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry-itest.cc@77 PS7, Line 77: ASSERT_TRUE(s.IsNotAuthorized()); add: << s.ToString(); http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry_advance-itest.cc File src/kudu/integration-tests/master_sentry_advance-itest.cc: PS7: Maybe, be more specific with the name of the file? If not, then at least rename master_sentry_advance-itest.cc --> master_sentry_advanced-itest.cc ? http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry_advance-itest.cc@40 PS7, Line 40: MasterAdvanceSentryTest MasterAdvancedSentryTest ? http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry_advance-itest.cc@57 PS7, Line 57: TEST_F(MasterAdvanceSentryTest, TestRenameTablePrivilegeTransfer) { : ASSERT_OK(GrantRenameTablePrivilege(kDatabaseName, kTableName)); : ASSERT_OK(RenameTable(Substitute("$0.$1", kDatabaseName, kTableName), : Substitute("$0.$1", kDatabaseName, "b"))); : NO_FATALS(CheckTable(kDatabaseName, "b", make_optional<const string &>(kAdminUser))); : : // TODO(hao): uncomment the following tests after SENTRY-2471 is fixed. : // Checks table rename in the HMS is synchronized with the Sentry privileges. : : // table_alterer.reset(client_->NewTableAlterer(Substitute("$0.$1", kDatabaseName, "b")) : // ->DropColumn("int16_val")); : : // Note that unlike table creation, there could be a delay between the table renaming : // in Kudu and the privilege renaming in Sentry. Because Kudu uses the transactional : // listener of the HMS to get notification of table alteration events, while Sentry : // uses post event listener (which is executed outside the HMS transaction). There : // is a chance that Kudu already finish the table renaming but the privilege renaming : // hasn't been reflected in the Sentry service. : // ASSERT_EVENTUALLY([&] { : // ASSERT_OK(table_alterer->Alter()); : // }); : // NO_FATALS(CheckTable(kDatabaseName, "b", make_optional<const string&>(kAdminUser))); : // : // table_alterer.reset(client_->NewTableAlterer(Substitute("$0.$1", kDatabaseName, "b")) : // ->RenameTo(Substitute("$0.$1", kDatabaseName, "c"))); : // ASSERT_OK(table_alterer->Alter()); : // NO_FATALS(CheckTable(kDatabaseName, "c", make_optional<const string&>(kAdminUser))); : } Would it be easier just to add the special DISABLED_ (TestRenameTablePrivilegeTransfer --> DISABLED_TestRenameTablePrivilegeTransfer) prefix to the name of the test and have the code as is, so when SENTRY-2471 is fixed, just drop the prefix? http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/master_sentry_advance-itest.cc@95 PS7, Line 95: Substitute("$0.$1", kDatabaseName, "non_existent"), : Substitute("$0.$1", kDatabaseName, "b")); here and in this file: introduce const varibles for the result of Substitute() and use them instead of calling Substitute() multiple times on the same data? http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/sentry_itest-base.h File src/kudu/integration-tests/sentry_itest-base.h: http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/sentry_itest-base.h@54 PS7, Line 54: using sentry::TSentryGrantOption; : using sentry::TSentryPrivilege; : : namespace kudu { : : using boost::make_optional; : using client::KuduScanToken; : using client::KuduScanTokenBuilder; : using client::KuduTableAlterer; : using cluster::ExternalMiniClusterOptions; : using hms::HmsClient; : using master::MasterServiceProxy; : using sentry::SentryClient; : using master::TabletLocationsPB; : using std::string; : using std::unique_ptr; : using std::vector; : using strings::Substitute; In general, adding 'using' into header files is not a good idea. It also contradicts the google's style guide. http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/sentry_itest-base.h@298 PS7, Line 298: std::function<Status(SentryTestBase*, const string&, const string&)> Introduce an intermediate typedef for this and use it to define OperatorFunc and PrivilegeFunc below? http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/integration-tests/sentry_itest-base.h@313 PS7, Line 313: table Is this name or id of the table? Consider naming the fields unambiguously or add comments. http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.cc@4644 PS7, Line 4644: restricted restrictive http://gerrit.cloudera.org:8080/#/c/11797/7/src/kudu/master/catalog_manager.cc@4651 PS7, Line 4651: TableMetadataLock table_lock(table.get(), LockMode::READ); The 'table' variable is not used elsewhere, and tablet_info is staying here anyways. So, maybe just TableMetadataLock table_lock(tablet_info->table().get(), LockMode::READ); ? -- To view, visit http://gerrit.cloudera.org:8080/11797 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iab4aa027ae6eb4520db48ce348db552c9feec2a8 Gerrit-Change-Number: 11797 Gerrit-PatchSet: 7 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 08 Feb 2019 05:39:18 +0000 Gerrit-HasComments: Yes
