Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12833 )
Change subject: [master] introduce SentryPrivilegesFetcher ...................................................................... Patch Set 13: (10 comments) > Patch Set 12: Code-Review+1 > > (6 comments) 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@182 PS9, Line 182: rivileges_cach > Ah, missed the fact that it'd be recursive naming. Yep, the function look-up rules in C++ turned to be non-intuitive in this particular case. I initially wrote that without namespace specifiers and got that compiler error. http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider-test.cc@426 PS9, Line 426: for (const auto& action : SelectRandomSubset<SentryActionsSet, SentryAction::Action, Random>( : kAllActions, 0, &prng)) { : > Sorry for the conflicts. Thanks for being accommodating. np, thanks for putting up a path to address that feedback. It seems I kept this patch around for too long, so I should have expected something like that anyways :) http://gerrit.cloudera.org:8080/#/c/12833/12/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/12833/12/src/kudu/master/sentry_authz_provider-test.cc@697 PS12, Line 697: caching > nit: Cache? Done http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider.cc@78 PS9, Line 78: s = privileges_branch.privileges(); > But I think this is confusing because this means that "privilege" would refer > to both the struct itself and a member internal to the struct. And so a clear > way to distinguish these two "privileges" is to call the member "privilege" > "action" instead. Continuing this bike-shedding venue, I could propose to rename AuthorizablePrivileges into AuthorizableAccessInfo or AuthorizableAccessRules. However, I think we need this patch to land eventually, so I hope renaming this into 'allowed_action' will be a reasonable solution :) http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider.cc@182 PS9, Line 182: > SGTM Great, thanks for being open for this alternative naming approach. http://gerrit.cloudera.org:8080/#/c/12833/12/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/12833/12/src/kudu/master/sentry_authz_provider.cc@50 PS12, Line 50: // action ('required_action') in the specified scope ('required_scope') > nit: missing ' Done http://gerrit.cloudera.org:8080/#/c/12833/12/src/kudu/master/sentry_privileges_fetcher.h File src/kudu/master/sentry_privileges_fetcher.h: http://gerrit.cloudera.org:8080/#/c/12833/12/src/kudu/master/sentry_privileges_fetcher.h@194 PS12, Line 194: Send > nit: Sends. Done http://gerrit.cloudera.org:8080/#/c/12833/12/src/kudu/master/sentry_privileges_fetcher.h@201 PS12, Line 201: Reset > nit: Resets Done 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: std::string table_name; > Thanks for doing that Alexey. np :) probably we will come up with something better in next revisions while iterating on the signatures of methods used as interface between SentryAuthzProvider and SentryAuthzFetcher. http://gerrit.cloudera.org:8080/#/c/12833/12/src/kudu/master/sentry_privileges_fetcher.cc File src/kudu/master/sentry_privileges_fetcher.cc: http://gerrit.cloudera.org:8080/#/c/12833/12/src/kudu/master/sentry_privileges_fetcher.cc@219 PS12, Line 219: i > Nit: got two spaces here. Done -- 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: 13 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 22:20:31 +0000 Gerrit-HasComments: Yes
