Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12833 )
Change subject: WIP [master] introduced SentryAuthzCache ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/12833/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12833/2//COMMIT_MSG@7 PS2, Line 7: WIP [master] introduced SentryAuthzCache Any thoughts on tackling the thundering herd calls to Sentry in this patch? Or perhaps that'll come in a following patch? Given the frequency of DML compared to DDL, I think that's the scenario that would benefit the most frequently from having a caching layer. http://gerrit.cloudera.org:8080/#/c/12833/2/src/kudu/master/sentry_authz_cache.h File src/kudu/master/sentry_authz_cache.h: http://gerrit.cloudera.org:8080/#/c/12833/2/src/kudu/master/sentry_authz_cache.h@56 PS2, Line 56: Status Fetch(thrift::HaClient<sentry::SentryClient>* ha_client, : const std::string& username, : const ::sentry::TSentryAuthorizable& object, : ::sentry::TListSentryPrivilegesResponse* response); Is synchronization built into TTLCache? Do we need to worry about concurrency in this class? http://gerrit.cloudera.org:8080/#/c/12833/2/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/12833/2/src/kudu/master/sentry_authz_provider-test.cc@104 PS2, Line 104: return sentry_client_->DropRole(role_req) : .AndThen([&]() { : return sentry_authz_provider_->NotifyAuthzInfoUpdated(); nit: why not RETURN_NOT_OK(sentry_client_->DropRole(role_req); return sentry_authz_provider_->NotifyAuthzInfoUpdated(); ? In general, I think of AndThen as a mechanism to handle errors of many different calls the same way, e.g. if there were some HandleError(s) to be called on the first bad Status in a series of Status-returning calls. In cases where this is trivial, e.g. in this case, the "handling" is just returning from the function, RETURN_NOT_OK is sufficient, no? http://gerrit.cloudera.org:8080/#/c/12833/2/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/12833/2/src/kudu/master/sentry_authz_provider.h@105 PS2, Line 105: // Test-only: notify the provider that the authz information has been updated : // at the source side (i.e. Sentry). Could you elaborate on what this means? Why do we need a method for this (even if it's test-only)? Why not call ResetCache() directly? http://gerrit.cloudera.org:8080/#/c/12833/2/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/12833/2/src/kudu/master/sentry_authz_provider.cc@120 PS2, Line 120: 0 means " : "'do not use cache any Sentry responses'. nit: reword as, "0 means Sentry responses will not be cached." http://gerrit.cloudera.org:8080/#/c/12833/2/src/kudu/master/sentry_authz_provider.cc@125 PS2, Line 125: 30, I commented on the design doc about this, but maybe consider making the default 1, or taking out this configuration completely? I don't see a compelling reason to have this be configurable (though I could be swayed). -- 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: 2 Gerrit-Owner: 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: Sat, 23 Mar 2019 00:48:03 +0000 Gerrit-HasComments: Yes
