Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12833 )
Change subject: WIP [master] introduced SentryPrivilegesFetcher ...................................................................... Patch Set 9: (2 comments) The renaming of sentry_authz_privilege_fetcher.{h,cc} made the new files in revisions after PS6 show up as new code, which made the review pretty frustrating. http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider-test.cc@426 PS9, Line 426: bool KerberosEnabled() const override { : return std::get<1>(GetParam()); : } > I chatted with Hao about this briefly, and it seems like non-Kerberos isn't Yeah I had the same feedback in Andrew's patch. In my case, I didn't comment on whether non-Kerberos is common and/or interesting, just that there didn't seem to be any intersection between Kerberos functionality and Sentry functionality (i.e. the former is used for authn and the latter for authz). Certainly from a _product_ perspective we want users to enable authn (otherwise their authz can be easily compromised), but from an integration testing perspective, I think we'd be better off only testing the matrix where we expect functional interactions. http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_privileges_fetcher.h File src/kudu/master/sentry_privileges_fetcher.h: http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_privileges_fetcher.h@100 PS9, Line 100: // A representation of the Sentry privilege hierarchy branch for a single table I left some feedback in Andrew's patch about this comment: I think this invariant would be clearer if enforced by SentryPrivilegesBranch itself. Right now that's not the case; it's expected that when a SentryPrivilegesBranch is constructed, the person constructing it remembers to enforce the invariant. -- 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: 9 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: Mon, 08 Apr 2019 05:14:46 +0000 Gerrit-HasComments: Yes