Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13069 )
Change subject: [authz] new SentryAuthzProvider's caching strategy ...................................................................... Patch Set 7: (4 comments) http://gerrit.cloudera.org:8080/#/c/13069/6/src/kudu/master/sentry_privileges_fetcher.h File src/kudu/master/sentry_privileges_fetcher.h: http://gerrit.cloudera.org:8080/#/c/13069/6/src/kudu/master/sentry_privileges_fetcher.h@126 PS6, Line 126: : // Add/merge privileges from other instance of SentryPrivilegesBranch. : void Merge(const SentryPrivilegesBranch& other); : : // Output the privileges into branches corresponding to DB-and-higher and : // TABLE-and-lower authz scopes. : void Split(SentryPrivilegesBranch* other_scope_db, : SentryPrivilegesBranch* other_scope_table) const; > Ah, I missed that in both cases we're operating on cached objects. Agreed t Ack http://gerrit.cloudera.org:8080/#/c/13069/6/src/kudu/master/sentry_privileges_fetcher.cc File src/kudu/master/sentry_privileges_fetcher.cc: http://gerrit.cloudera.org:8080/#/c/13069/6/src/kudu/master/sentry_privileges_fetcher.cc@422 PS6, Line 422: switch (e.scope) { > Mike and I talked about emplace_back (without std::move) vs. push_back in a Done http://gerrit.cloudera.org:8080/#/c/13069/7/src/kudu/master/sentry_privileges_fetcher.cc File src/kudu/master/sentry_privileges_fetcher.cc: http://gerrit.cloudera.org:8080/#/c/13069/7/src/kudu/master/sentry_privileges_fetcher.cc@534 PS7, Line 534: t expected to receive any requests > nit: "receive any requests" makes it sound like this is a service and that Done. Nice find about IsGTest(). I'm not sure using it here brings a lot of value, since that's kind of a reverse case: it would make sense to use it if there were no call with the SERVER scope in tests, so we could safely crash when spotting 'a programming error' in that imaginary case. http://gerrit.cloudera.org:8080/#/c/13069/7/src/kudu/master/sentry_privileges_fetcher.cc@538 PS7, Line 538: received request to fetch privileges of the SERVER scope " : "on authorizable '$0' for user '$1'", table_name, user); > nit: same here re "received request", maybe reword as "requesting to fetch Done -- To view, visit http://gerrit.cloudera.org:8080/13069 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id96181345e357a104e28314d8d8d88633dcf9608 Gerrit-Change-Number: 13069 Gerrit-PatchSet: 7 Gerrit-Owner: Alexey Serbin <[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: Wed, 24 Apr 2019 04:16:27 +0000 Gerrit-HasComments: Yes
