Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12941 )
Change subject: sentry: generate table privileges ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/12941/2/src/kudu/master/default_authz_provider.h File src/kudu/master/default_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/12941/2/src/kudu/master/default_authz_provider.h@62 PS2, Line 62: Status GetTablePrivilegePB(const std::string& /*table_name*/, Don't we need to set pb->table_id also? Given the function signature, I'd expect it to write out 'pb' from scratch; if that's not the case, the contract should be clarified in a comment. http://gerrit.cloudera.org:8080/#/c/12941/2/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/12941/2/src/kudu/master/sentry_authz_provider.cc@554 PS2, Line 554: TablePrivilegePB privilege_pb; Should set table_id, right? http://gerrit.cloudera.org:8080/#/c/12941/2/src/kudu/master/sentry_authz_provider.cc@555 PS2, Line 555: unordered_set<string> scannable_col_names; Seems like the transformations that happen in the rest of this function could happen in GetSentryPrivileges and thus be cached. What's the rationale for doing them for every token retrieval vs. doing them earlier and benefitting from caching? http://gerrit.cloudera.org:8080/#/c/12941/2/src/kudu/master/sentry_authz_provider.cc@558 PS2, Line 558: SentryAuthorizableScope(SentryAuthorizableScope::TABLE))) { This can be constructed once outside the loop. http://gerrit.cloudera.org:8080/#/c/12941/2/src/kudu/master/sentry_authz_provider.cc@581 PS2, Line 581: else if (ContainsKey(privilege.granted_privileges, SentryAction::ALL) || : ContainsKey(privilege.granted_privileges, SentryAction::OWNER) || : ContainsKey(privilege.granted_privileges, SentryAction::SELECT)) { : // Pull out any scan privileges at the column scope. : DCHECK_EQ(SentryAuthorizableScope::COLUMN, privilege.scope); : DCHECK(!privilege.column_name.empty()); : EmplaceIfNotPresent(&scannable_col_names, privilege.column_name); : } We can skip this if an earlier privilege set privilege_pb.scan_privilege(), right? http://gerrit.cloudera.org:8080/#/c/12941/2/src/kudu/master/sentry_authz_provider.cc@599 PS2, Line 599: *pb = std::move(privilege_pb); Note that this implementation _overwrites_ pb while the default implementation modifies it in situ. Could you make them consistent and clarify the intended behavior in the contract? -- To view, visit http://gerrit.cloudera.org:8080/12941 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f Gerrit-Change-Number: 12941 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 08 Apr 2019 04:56:38 +0000 Gerrit-HasComments: Yes
