----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46909/#review132962 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java (line 74) <https://reviews.apache.org/r/46909/#comment197249> throw an exception. SentryConfigurationException? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java (line 85) <https://reviews.apache.org/r/46909/#comment197250> throw a RunTimeException (are all those runtimeexceptions already?) sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java (line 92) <https://reviews.apache.org/r/46909/#comment197251> throw an exception sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java (line 111) <https://reviews.apache.org/r/46909/#comment197252> this can be a warn I think, only really need to error if we need to revoke privileges sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java (line 125) <https://reviews.apache.org/r/46909/#comment197254> error here. sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java (line 235) <https://reviews.apache.org/r/46909/#comment197257> this is backwards. Build up the table internally without mention of a TableCache, then once you are done, create a TableCache with that table. sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java (line 219) <https://reviews.apache.org/r/46909/#comment197244> won't the solr tests fail if you don't do the same thing? sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/TableCache.java (line 46) <https://reviews.apache.org/r/46909/#comment197245> this should just be an interface with getCache defined IMO. By declaring this volatile, you are saying everything needs to pay a performance penalty because some caches are updatable. But you already have an updatable cache. This means you can't use this effectively for the SimpleFileProviderBackend, which is one of things we want to do in the future. sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/TableCache.java (line 53) <https://reviews.apache.org/r/46909/#comment197246> this doesn't need to be in the interface, again, this is something the UpdatableCache needs, not the TableCache. sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/TableCache.java (line 61) <https://reviews.apache.org/r/46909/#comment197247> same here. sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/TableCache.java (line 70) <https://reviews.apache.org/r/46909/#comment197248> same here. - Gregory Chanan On May 12, 2016, 9:03 a.m., Ashish Singh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46909/ > ----------------------------------------------------------- > > (Updated May 12, 2016, 9:03 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/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 > 3c0e349443e3917812b46aa4547c3cab14dab052 > > 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 > >
