----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46909/#review133194 -----------------------------------------------------------
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/TableCache.java (line 21) <https://reviews.apache.org/r/46909/#comment197473> 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. sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java (line 62) <https://reviews.apache.org/r/46909/#comment197475> 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? sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java (line 72) <https://reviews.apache.org/r/46909/#comment197476> just make this a non-final TableCache and set it once initialize is done. sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java (line 231) <https://reviews.apache.org/r/46909/#comment197481> don't use the cache like this, because it violates the sensible constraint that the cache isn't modified externally. If you want to minimize the logic change here, probably the eaiest way is just to declare another table to hold the merges in parse() and pass that in here. Then, when parse or initialize is done, create the cache with that table, e.g. this.cache = new TableCache() { @Override public Table<> getCache() { return myTableWithMergedResults; } } sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java (line 331) <https://reviews.apache.org/r/46909/#comment197478> just return the internal cache here, get rid of getCache() function. - Gregory Chanan 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 > >
