Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12833 )
Change subject: WIP [master] introduced SentryPrivilegesFetcher ...................................................................... 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 Whoops, I thought I fixed those descriptions. Done. 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. Done http://gerrit.cloudera.org:8080/#/c/12833/6/src/kudu/master/sentry_authz_privilege_fetcher.h@105 PS6, Line 105: Status CoarsenAuthzScope( > Doc this? Done 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->privilege Yep, sure -- moved to the right place. 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? Whoops, my bad. I thought if I missed those tidy will catch it, but it seems that's not the case. 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: > Yeah I guess it is, though I think it's a little clearer doing it in the lo Done http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider.cc@524 PS5, Line 524: > I was referring specifically to the DHCECK_EQ: no matter what the authoriza Ah, sure. That's redundant; I removed it. Probably, I thought '10 lines above' were lines with those 'if ()' scopes. -- 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: Wed, 03 Apr 2019 05:28:42 +0000 Gerrit-HasComments: Yes
