Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12833 )
Change subject: WIP [master] introduced SentryPrivilegesFetcher ...................................................................... Patch Set 11: (12 comments) http://gerrit.cloudera.org:8080/#/c/12833/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12833/11//COMMIT_MSG@7 PS11, Line 7: WIP This is no longer a WIP? http://gerrit.cloudera.org:8080/#/c/12833/11//COMMIT_MSG@10 PS11, Line 10: raw responses : received from Sentry We actually store sanitized response from Sentry (SentryPrivilegesBranch), right? http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_authz_provider.h@98 PS11, Line 98: GetSentryPrivileges nit: it looks like the comment is removed? http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_authz_provider.cc@44 PS11, Line 44: DoPrivilegesImplyAction Maybe rename it to 'PrivilegeBranchImplies' to state that this is 'Whether the given privilege imply the one from the same branch but with the specified action at the specified scope with 'GRANT ALL' option (if any).' http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_authz_provider.cc@182 PS11, Line 182: fetcher_.GetSentryPrivileges nit: use SentryAuthzProvider::GetSentryPrivileges? http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_privileges_fetcher.h File src/kudu/master/sentry_privileges_fetcher.h: http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_privileges_fetcher.h@92 PS11, Line 92: granted_privileges Not your fault, but I think here name it as granted_actions is more appropriate. http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_privileges_fetcher.h@183 PS11, Line 183: Transform nit: Transforms. http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_privileges_fetcher.h@189 PS11, Line 189: Set nit: Sets, and the below. http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_privileges_fetcher.h@200 PS11, Line 200: test-only method Is this still a test-only method, I saw it has been used in Start()? http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_privileges_fetcher.h@207 PS11, Line 207: Broaden the authz scope Can you comment on how scope_depth_limit_ is used in this method and why it is used like that? http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_privileges_fetcher.cc File src/kudu/master/sentry_privileges_fetcher.cc: http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_privileges_fetcher.cc@224 PS11, Line 224: const boost::optional<SentryAuthorizableScope>& scope_limit = boost::none It doesn't seem we are using 'scope_limit' in this patch, and I am not sure if we need it in the further cache optimization patch. If not, should it be removed? http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_privileges_fetcher.cc@233 PS11, Line 233: futher nit: further -- 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: 11 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, 10 Apr 2019 06:05:23 +0000 Gerrit-HasComments: Yes
