Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13069 )
Change subject: [authz] new SentryAuthzProvider's caching strategy ...................................................................... Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/13069/2/src/kudu/master/sentry_privileges_fetcher.cc File src/kudu/master/sentry_privileges_fetcher.cc: http://gerrit.cloudera.org:8080/#/c/13069/2/src/kudu/master/sentry_privileges_fetcher.cc@541 PS2, Line 541: auto handle = cache_->Get(e); : if (!handle) { : continue; : } : handles.emplace_back(std::move(handle)); > We're now doing Get() in a loop. Does this mean that we might go to Sentry No, it does not -- those are queries to the cache. http://gerrit.cloudera.org:8080/#/c/13069/2/src/kudu/master/sentry_privileges_fetcher.cc@548 PS2, Line 548: if (!handles.empty()) { : SentryPrivilegesBranch result; : for (const auto& e : handles) { : DCHECK(e); : result.Merge(e.value()); : } : *privileges = std::move(result); : return Status::OK(); : } > Yeah this is a bug: This is addressed now. http://gerrit.cloudera.org:8080/#/c/13069/2/src/kudu/master/sentry_privileges_fetcher.cc@610 PS2, Line 610: requested_scope >= SentryAuthorizableScope::TABLE > This seems to be tied to the implementation/order of the enums instead of t Yeah, but it makes sense to keep in that order that reflect the hierarchy. I'm not sure Implies() is clearer, at least for me Implies() is much more obscure. IMO, in the context of PrivilegeFetcher it's easier to reason about hierarchy in terms of level. -- 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: 2 Gerrit-Owner: Alexey Serbin <aser...@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-Comment-Date: Sat, 20 Apr 2019 02:19:07 +0000 Gerrit-HasComments: Yes
