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