----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46909/#review132161 -----------------------------------------------------------
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/Cache.java (line 15) <https://reviews.apache.org/r/46909/#comment196313> I'd just combine this and TableCache, not obvious we need both. sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/CacheProvider.java (line 23) <https://reviews.apache.org/r/46909/#comment196314> Shouldn't this be CacheProviderBackend and extend ProviderBackend? Just have it skip validation or maybe even better throw an exception if someone tries to validate e.g. "Cache itself does not validate, that is the responsibility of the builder of the cache." sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java (line 35) <https://reviews.apache.org/r/46909/#comment196319> You shouldn't need a specific type of convertor here. Have the binding set the convertor type (or use a convertor factory) and use reflection here. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java (line 46) <https://reviews.apache.org/r/46909/#comment196318> See the comments in SimpleFileProviderBackend, I think it's cleaner if you just use a CacheProviderBackend instead of dealing with derivation. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java (line 49) <https://reviews.apache.org/r/46909/#comment196320> you shouldn't need direct access to the cache here. Use a CacheProviderBackend. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java (line 35) <https://reviews.apache.org/r/46909/#comment196321> Just use TimeUnit conversion. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java (line 40) <https://reviews.apache.org/r/46909/#comment196322> final sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java (line 41) <https://reviews.apache.org/r/46909/#comment196323> final sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java (line 42) <https://reviews.apache.org/r/46909/#comment196324> final sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java (line 43) <https://reviews.apache.org/r/46909/#comment196325> final sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java (line 44) <https://reviews.apache.org/r/46909/#comment196328> move this so all the final variables are together and all the non-final variables are together. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java (line 45) <https://reviews.apache.org/r/46909/#comment196326> final sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java (line 46) <https://reviews.apache.org/r/46909/#comment196327> final sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java (line 102) <https://reviews.apache.org/r/46909/#comment196330> I think this is okay because the ProviderBackend does it to, but at some point we may want to not create a new client each time. Maybe just add a comment. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java (line 115) <https://reviews.apache.org/r/46909/#comment196329> did you want to deal with the fact that the privilege convertor are probably validating? Maybe the nicest way to do that is to take a factory in the generic provider backend with a boolean for validation or whatever. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java (line 128) <https://reviews.apache.org/r/46909/#comment196334> This function doesn't really do anything, just get rid of it. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java (line 139) <https://reviews.apache.org/r/46909/#comment196332> Can Timertasks scheduled at fixed rate run concurrently ? It doesn't seem like it to me given the documentation says " two or more executions will occur in rapid succession to "catch up." " If so, the semaphore usage seems unnecessary. Just take a boolean in startUpdateThread on whether you should block until the initial update in complete. You can just call loadAllData directly if that is true. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java (line 176) <https://reviews.apache.org/r/46909/#comment196335> Your use of cache isn't thread safe, see http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/collect/HashBasedTable.html "Note that this implementation is not synchronized. If multiple threads access this table concurrently and one of the threads modifies the table, it must be synchronized externally." Probably the easiest way to do this is to ensure that you aren't modifying the table externally (there are some places above where you are), then you never modify the table once it's built, you only swap out the table. So cache.clear would just be cache = new Table<>... You still need to synchronize on the table object itself, probably the easiest way to do that is just make it volatile. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java (line 177) <https://reviews.apache.org/r/46909/#comment196331> make this a warn or error sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java (line 182) <https://reviews.apache.org/r/46909/#comment196333> I think reloadCache is a better name. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java (line 237) <https://reviews.apache.org/r/46909/#comment196317> Not obvious what this means. CACHE_UPDATE_FAILURES_BEFORE_PRIV_REVOKE? sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java (line 61) <https://reviews.apache.org/r/46909/#comment196315> I think it's cleaner if you just use the CacheProvider(Backend) instead of deriving. Prefer composition over inheritance. sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java (line 233) <https://reviews.apache.org/r/46909/#comment196316> you shouldn't be writting the cache directly, just build the temp table and create the cache + cache provider backend with that. Goes with the prefer composition over inheritance comment above. - Gregory Chanan On May 8, 2016, 1:15 a.m., Ashish Singh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46909/ > ----------------------------------------------------------- > > (Updated May 8, 2016, 1:15 a.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/Cache.java > PRE-CREATION > > 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-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 > 3c0e349443e3917812b46aa4547c3cab14dab052 > > sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/TableCache.java > PRE-CREATION > > 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 > >
