> 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.
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.
- Gregory
-----------------------------------------------------------
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
>
>
