> On May 6, 2016, 10 p.m., Gregory Chanan wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java, > > line 95 > > <https://reviews.apache.org/r/46909/diff/3/?file=1374941#file1374941line95> > > > > hmm...the SimpleFileProviderBackend doesn't do this either? Do you > > know what's up with that?
Nope, as this is coming form cache, filtering does not buy a lot in terms of perf. However, we might want to add this for consistency reasons, have a follow up JIRA on that. > On May 6, 2016, 10 p.m., Gregory Chanan wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java, > > line 90 > > <https://reviews.apache.org/r/46909/diff/3/?file=1374941#file1374941line90> > > > > It's not great how the code (and bugs, see below) with > > SimpleFileProviderBackend is essentially duplicated here. Here is one way > > this would ideally look (not subject to compatibility constraits). > > > > 1) Define a "Cache" interface with a single function: getCache > > 2) Split out the getPrivileges / getRoles implementations from > > SimpleFileProviderBackend to something like CacheProviderBackend that takes > > a Cache object > > 3) SimpleFileProviderBackend then uses a CacheProviderBackend that > > always returns the table built in initialize > > 4) The Generic backend, when caching is enabled, uses a > > CacheProviderBackend that is identical to your cache here. I.e. the Cache > > here just implements getCache in the way get() is implemented. > > > > 5) Ideally, the cache-enabled and non-cache enabled backends here would > > be two completely separate types. The usual way of accomplishing that is > > via a factory; I don't know if that's feasible because I think the > > configuration specifies the actual type (not a factory type) and calls the > > constructor directly. If you think a factory is a better option, I don't > > think you need to implement it now, but at least file a jira. Took care of most of it, filed SENTRY-1249 to track creating SentryGenericProviderBackend with factory. > On May 6, 2016, 10 p.m., Gregory Chanan wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java, > > line 211 > > <https://reviews.apache.org/r/46909/diff/3/?file=1374941#file1374941line211> > > > > with this implementation, all the requests at the beginning are going > > to fail, right? Should we just pause while we build the cache initially? > > Slower startup/responses seem desirible than everything initially failing. Added a timed check, if not completed in specified time, provider backend won't be initialized and users will see initialization failure, prompting them to look at potential causes of failure of cache building. - Ashish ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46909/#review132087 ----------------------------------------------------------- On May 6, 2016, 5:08 p.m., Ashish Singh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46909/ > ----------------------------------------------------------- > > (Updated May 6, 2016, 5:08 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 SentryGenericProviderBackend. > > > Diffs > ----- > > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java > ffd3af4903dd59f37ea1ff4a55138742fdfa74da > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java > 222b6fd530a0e26ea625d1be0fd68b0828558316 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/Updatable.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java > PRE-CREATION > > 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-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java > a2cfa28da2af548a872ec0d2e5620ae1c27041b3 > > sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/TestAuthorize.java > 250522ed33394a667b1a0d6c47c68b210e0214a3 > > Diff: https://reviews.apache.org/r/46909/diff/ > > > Testing > ------- > > Extended Kafka e2e tests. Will be running perf tests for Kafka-Sentry > integration with caching turned on. > > > Thanks, > > Ashish Singh > >
