> On May 8, 2016, 4:35 a.m., Gregory Chanan wrote: > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/CacheProvider.java, > > line 23 > > <https://reviews.apache.org/r/46909/diff/6/?file=1375790#file1375790line23> > > > > 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."
This is not an issue right, if we need to do this, we can do it in a follow up JIRA. > On May 8, 2016, 4:35 a.m., Gregory Chanan wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java, > > line 49 > > <https://reviews.apache.org/r/46909/diff/6/?file=1375792#file1375792line49> > > > > you shouldn't need direct access to the cache here. Use a > > CacheProviderBackend. Required to make sure we block on first cache loading. > On May 8, 2016, 4:35 a.m., Gregory Chanan wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java, > > line 35 > > <https://reviews.apache.org/r/46909/diff/6/?file=1375793#file1375793line35> > > > > Just use TimeUnit conversion. Nice, was not aware of it. Thanks! > On May 8, 2016, 4:35 a.m., Gregory Chanan wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java, > > line 115 > > <https://reviews.apache.org/r/46909/diff/6/?file=1375793#file1375793line115> > > > > 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. I think we can have this in a follow up JIRA. If that sounds OK to you I will file a JIRA. > On May 8, 2016, 4:35 a.m., Gregory Chanan wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java, > > line 176 > > <https://reviews.apache.org/r/46909/diff/6/?file=1375793#file1375793line176> > > > > 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. Thanks for pointing this out. > On May 8, 2016, 4:35 a.m., Gregory Chanan wrote: > > sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java, > > line 61 > > <https://reviews.apache.org/r/46909/diff/6/?file=1375797#file1375797line61> > > > > I think it's cleaner if you just use the CacheProvider(Backend) instead > > of deriving. Prefer composition over inheritance. Similar to somewhere above, not an issue, we can deal this refactoring in a follow up JIRA. - Ashish ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46909/#review132161 ----------------------------------------------------------- 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 > >
