> On May 2, 2016, 9:45 p.m., Gregory Chanan wrote:
> > Why put the caching at the client layer instead of the provider backend
> > layer? It seems unlikely to me that the average client actually wants
> > caching or need caching, whereas an arbitrary external service probably
> > does.
>
> Ashish Singh wrote:
> Any arbitrary external service will need a client, right? Can't think of
> a use case where a service would want to maintain a provider backend without
> creating a client. Maybe I don't have complete context. Mind providing some
> info here?
>
> Ashish Singh wrote:
> Never mind, I completely mixed up Client impl and provider backend :(.
> Yea, you are correct that it makes sense to have caching in provider backend
> instead. Will update.
>
> Ashish Singh wrote:
> Ok, I think I am running into circles now :), but with each round
> hopefully there will be more info. So, provider backend is going to use a
> client as well, right? Depending on what the provider wants, it can enable or
> disable caching. I think it is better to have a generic client impl with
> caching support, which can then be used by multiple provider backends than
> having one of the provider backend have caching support.
>
> Gregory Chanan wrote:
> You are right, I think implementing at the client level is reasonable. I
> don't think inserting the code into the DefaultImpl is a good idea, though,
> consider:
> 1) If I write another client, I'll have to write my own caching mechanism
> when there isn't really anything specific about your implementaiton
> 2) Whether I cache or not is sort of magic, i.e. there's no enforced
> guarantee that the GenericServiceClientFactory returns a default impl and
> thus no guarantee that the caching will be enforced.
>
> So, I think this needs two changes:
> 1) Define your own GenericServiceClient that has another
> GenericServiceClient and provides caching on top of it. In this way, any
> client can easily gain the ability to cache
> 2) You need some way of specifying that you want caching. For example,
> check that caching is enabled in the conf in the factory method and return a
> caching client if so.
>
> Gregory Chanan wrote:
> Actually, now that I've gone through and read the code, I'm not sure
> implementing at the client level is the best idea right now. I think your
> arguments for why a cache at the client level makes sense are totally valid,
> but in reality the client APIs are overly complicated right, and you have to
> implement each one. Specificially:
>
> 1) You need to implement all of the following APIs:
> Set<TSentryRole> listRolesByGroupName(
> String requestorUserName,
> String groupName,
> String component)
> throws SentryUserException;
>
> Set<TSentryRole> listUserRoles(String requestorUserName, String
> component)
> throws SentryUserException;
>
> Set<TSentryRole> listAllRoles(String requestorUserName, String
> component)
> throws SentryUserException;
>
> Set<TSentryPrivilege> listPrivilegesByRoleName(
> String requestorUserName, String roleName, String component,
> String serviceName, List<? extends Authorizable> authorizables)
> throws SentryUserException;
>
> Set<TSentryPrivilege> listPrivilegesByRoleName(
> String requestorUserName, String roleName, String component,
> String serviceName) throws SentryUserException;
>
> Set<String> listPrivilegesForProvider(String component,
> String serviceName, ActiveRoleSet roleSet, Set<String> groups,
> List<? extends Authorizable> authorizables) throws
> SentryUserException;
>
> when you had the code embedded in the DefaultImpl, you were able to
> reduce the number of cases by looking at the actual implementation of the
> DefaultImpl, e.g. some of these functions called other ones. But if you are
> implementing a generic cache, you can't depend on that behavior; my arbitrary
> client doesn't necessary have the same implement as the DefaultImpl.
>
> 2) It's difficult to cache these in such a way that makes sense and
> follows the API. For example, listPrivilegesByRoleName: you need to key your
> cache based on the combination of {roleName, component, serviceName} at a
> minimum as well as implement the logic for filtering based on authorizable.
> That in general doesn't look like it was done correctly in this review.
>
> 3) The client/cache interface doesn't make complete sense yet. E.g. I've
> defined a cache...what do you cache when I do dropRole? You'd really want
> different interfaces for read/write and be able to specify/pass in caches for
> the read behaviors.
>
> In contract, implementing this at the ProviderBackend level has none of
> these issues.
>
> 1) The ProviderBackend only has 3 functions to implement:
> ImmutableSet<String> getPrivileges(Set<String> groups, ActiveRoleSet
> roleSet, Authorizable... authorizableHierarchy);
>
> ImmutableSet<String> getPrivileges(Set<String> groups, Set<String> users,
> ActiveRoleSet roleSet, Authorizable... authorizableHierarchy);
>
> ImmutableSet<String> getRoles(Set<String> groups, ActiveRoleSet roleSet);
>
> and remarkably, these functions already have an implementation! Assuming
> you have a Role x Group x Privilege table (i.e. a cache), the
> SimpleFileProviderBackend already implements a ProviderBackend. You just
> need to separate out the initialize/validate code and you are done.
>
> 2) Again, it's trivial to implement the cache at the ProviderBackend
> level because of the SimpleFileProviderBackend implementation.
>
> 3) Since the ProviderBackend only has READ interfaces, these is no
> confusion about what a cache means.
I agree moving to provider backend will definitely provide the ease of
implementation and other benefits you have mentioned. I think you have
convinced me to move to provider backend. I will give it a try and hope that
nothing pops up to make me go back to the clients approach :). Thanks for the
valuabke feedback. Btw, I was so amazed by the way public APIs are defined and
docuemnted in Sentry that I could not stop myself from filing a super vague
JIRA, SENTRY-1214. I think it will be really worthwhile if some Sentry pros
spend some time to refactor Sentry's APIs.
- Ashish
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46909/#review131381
-----------------------------------------------------------
On May 2, 2016, 9:32 p.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46909/
> -----------------------------------------------------------
>
> (Updated May 2, 2016, 9:32 p.m.)
>
>
> Review request for sentry, Dapeng Sun, Gregory Chanan, Hao Hao, and Sravya
> Tirukkovalur.
>
>
> Bugs: SENTRY-1229
> https://issues.apache.org/jira/browse/SENTRY-1229
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1229: Add caching to SentryGenericServiceClientDefaultImpl.
>
>
> Diffs
> -----
>
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClient.java
> 60502895a0fcdd781f5bd61b29a676a7c96f81b8
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java
> dce3dade7f42fe35a849612c5caf2e98d2dac578
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
> 00e3fbde76ebf84704ee110adfca30869845a7b8
>
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericServiceIntegration.java
> fcf0e7b9dc45eb12cc1b76f3084efd6340c039a4
>
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericServiceIntegrationWithCaching.java
> PRE-CREATION
>
> Diff: https://reviews.apache.org/r/46909/diff/
>
>
> Testing
> -------
>
> Extended unit tests. Will be running perf tests for Kafka-Sentry integration
> with caching turned on.
>
>
> Thanks,
>
> Ashish Singh
>
>
