> On May 13, 2016, 8:38 p.m., Gregory Chanan wrote: > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/TableCache.java, > > line 22 > > <https://reviews.apache.org/r/46909/diff/9-11/?file=1380747#file1380747line22> > > > > nit: this comment is a little strange. Caller must not modify the > > returned cache, unless it is required? Why would you modify something if > > it wasn't required? Just say don't modify it -- I don't think > > SimpleFileProviderBackend actually needs to modify it.
I wanted to convey that if you are sure about what you are doing then only modify the table. There are definitely reasons where one would avoid want to avoid the copy and make changes to cache itself. Updating cache should be fine for eventual consistency for example. But, I see your point on how the comment can be ambiguous, removing the warning for now. > On May 13, 2016, 8:38 p.m., Gregory Chanan wrote: > > sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java, > > line 63 > > <https://reviews.apache.org/r/46909/diff/9-11/?file=1380753#file1380753line63> > > > > I don't think you need to implement a TableCache here -- you can just > > use it internally. There is no external caller that needs to use this as a > > cache, right? Sure, makes sense. - Ashish ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46909/#review133194 ----------------------------------------------------------- On May 13, 2016, 4:20 p.m., Ashish Singh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46909/ > ----------------------------------------------------------- > > (Updated May 13, 2016, 4:20 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/CacheProvider.java > PRE-CREATION > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java > ffd3af4903dd59f37ea1ff4a55138742fdfa74da > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/TableCache.java > PRE-CREATION > > 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/UpdatableCache.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClient.java > 76ff15b91fddad6b5aa429d2439eb2a5086544d7 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java > 74b6963ab4015521462fb3c25559d0d12292424f > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > 00e3fbde76ebf84704ee110adfca30869845a7b8 > > sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java > 079d52a07853fa897d54ef277fdf1e76ed5a82e5 > > 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 > >
