Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13549 )
Change subject: sentry: avoid authorizing every table in ListTables ...................................................................... Patch Set 3: (12 comments) http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/authz_provider.h File src/kudu/master/authz_provider.h: http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/authz_provider.h@28 PS3, Line 28: #include "kudu/master/catalog_manager.h" > Why do we need this? Can't we forward declare TableInfo? Done http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/default_authz_provider.h File src/kudu/master/default_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/default_authz_provider.h@32 PS3, Line 32: class scoped_refptr; > warning: invalid case style for class 'scoped_refptr' [readability-identifi Ack http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/sentry_authz_provider-test.cc@400 PS3, Line 400: . > Nit: extra period here. Done http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/sentry_authz_provider-test.cc@401 PS3, Line 401: const string dummy_name = Substitute("$0_$1", "foo", d); : if (FLAGS_has_db_privileges) { : ASSERT_OK(AlterRoleGrantPrivilege(GetDatabasePrivilege(db_name, "METADATA"))); : } else { : ASSERT_OK(AlterRoleGrantPrivilege(GetDatabasePrivilege(dummy_name, "METADATA"))); : } > Rewrite: Done http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/sentry_authz_provider-test.cc@409 PS3, Line 409: ,d > Nit: , d Refactored away http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/sentry_authz_provider.cc@204 PS3, Line 204: map<string, vector<std::pair<string, string>>> tables_by_db; > Could be unordered_map, unless you want alphanumeric ordering for the METAD Done http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/sentry_authz_provider.cc@210 PS3, Line 210: TableMetadataLock ltm(table_info.get(), LockMode::READ); > I am onboard with this, though I hope we can continue to separate catalog m Done http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/sentry_authz_provider.cc@233 PS3, Line 233: const string& first_table_name_in_db = tables_in_db[0].first; > Does this need to be deterministic? Like, should we sort tables_in_db or so No, added a comment. http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/sentry_authz_provider.cc@234 PS3, Line 234: if (user) { > Is this check correct? Previously we would include all tables in the respon Done http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/sentry_authz_provider.cc@235 PS3, Line 235: scoped_refptr<TableInfo> table_info; > Unused. Done http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/sentry_authz_provider.cc@239 PS3, Line 239: first_table_name_in_db > I think I know why you're providing the table name and not database name he Done http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/sentry_authz_provider.cc@253 PS3, Line 253: ListTablesResponsePB::TableInfo* table = pb->add_tables(); : table->set_name(name_and_id.first); : table->set_id(name_and_id.second); > May be able to save a few LOC with a lambda. Refactored away -- To view, visit http://gerrit.cloudera.org:8080/13549 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7c495c635fbd2a661709b6d7f04973b7864b5527 Gerrit-Change-Number: 13549 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong <aw...@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: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Sat, 08 Jun 2019 08:30:24 +0000 Gerrit-HasComments: Yes
