Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12833 )
Change subject: WIP [master] introduced SentryAuthzCache ...................................................................... Patch Set 6: (7 comments) http://gerrit.cloudera.org:8080/#/c/12833/6/src/kudu/master/sentry_authz_privilege_cache_metrics.cc File src/kudu/master/sentry_authz_privilege_cache_metrics.cc: http://gerrit.cloudera.org:8080/#/c/12833/6/src/kudu/master/sentry_authz_privilege_cache_metrics.cc@42 PS6, Line 42: found find http://gerrit.cloudera.org:8080/#/c/12833/6/src/kudu/master/sentry_authz_privilege_fetcher.h File src/kudu/master/sentry_authz_privilege_fetcher.h: http://gerrit.cloudera.org:8080/#/c/12833/6/src/kudu/master/sentry_authz_privilege_fetcher.h@46 PS6, Line 46: class SentryAuthzPrivilegeFetcher { Please doc the class and its public methods. This may be a good place to describe the hierarchy and "coarsening"/"shallowing". http://gerrit.cloudera.org:8080/#/c/12833/6/src/kudu/master/sentry_authz_privilege_fetcher.h@105 PS6, Line 105: Status CoarsenAuthzScope( Doc this? http://gerrit.cloudera.org:8080/#/c/12833/6/src/kudu/master/sentry_authz_privilege_fetcher.cc File src/kudu/master/sentry_authz_privilege_fetcher.cc: http://gerrit.cloudera.org:8080/#/c/12833/6/src/kudu/master/sentry_authz_privilege_fetcher.cc@257 PS6, Line 257: // This is a simple approximation: the exact information could be available : // from the allocator of this std::set() instance, but public API of : // std::allocator doesn't provide such information. Nit: this is true, but it only applies to the measurement of obj->privileges; not of obj or of the capacity of each string in the obj->privileges object set. http://gerrit.cloudera.org:8080/#/c/12833/6/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/12833/6/src/kudu/master/sentry_authz_provider.h@98 PS6, Line 98: static bool HasPrivilege( These methods no longer exist, right? http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider.cc@369 PS5, Line 369: > I think that's already accounted at line 368, no? Yeah I guess it is, though I think it's a little clearer doing it in the loop (since you don't need to worry about whether obj->privileges contains objects of type TSentryPrivilege or something else). http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider.cc@524 PS5, Line 524: > Not necessarily. This verification is meant to spot anomalies like the fol I was referring specifically to the DHCECK_EQ: no matter what the authorizable/privilege contain, 'a' and 'p' will always be 4 elements long each. Am I missing something? -- To view, visit http://gerrit.cloudera.org:8080/12833 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idaefacd50736f1f152dae34e76778e17b2e84cbe Gerrit-Change-Number: 12833 Gerrit-PatchSet: 6 Gerrit-Owner: Alexey Serbin <aser...@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: Tue, 02 Apr 2019 03:43:22 +0000 Gerrit-HasComments: Yes
