----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46909/#review132087 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java (line 90) <https://reviews.apache.org/r/46909/#comment196186> It's not great how the code (and bugs, see below) with SimpleFileProviderBackend is essentially duplicated here. Here is one way this would ideally look (not subject to compatibility constraits). 1) Define a "Cache" interface with a single function: getCache 2) Split out the getPrivileges / getRoles implementations from SimpleFileProviderBackend to something like CacheProviderBackend that takes a Cache object 3) SimpleFileProviderBackend then uses a CacheProviderBackend that always returns the table built in initialize 4) The Generic backend, when caching is enabled, uses a CacheProviderBackend that is identical to your cache here. I.e. the Cache here just implements getCache in the way get() is implemented. 5) Ideally, the cache-enabled and non-cache enabled backends here would be two completely separate types. The usual way of accomplishing that is via a factory; I don't know if that's feasible because I think the configuration specifies the actual type (not a factory type) and calls the constructor directly. If you think a factory is a better option, I don't think you need to implement it now, but at least file a jira. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java (line 95) <https://reviews.apache.org/r/46909/#comment196182> hmm...the SimpleFileProviderBackend doesn't do this either? Do you know what's up with that? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java (line 211) <https://reviews.apache.org/r/46909/#comment196183> with this implementation, all the requests at the beginning are going to fail, right? Should we just pause while we build the cache initially? Slower startup/responses seem desirible than everything initially failing. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/Updatable.java (line 21) <https://reviews.apache.org/r/46909/#comment196180> Comment about what this is useful for? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/Updatable.java (line 28) <https://reviews.apache.org/r/46909/#comment196222> just make this a constructor param (or builder)? Don't see the need to change this in the middle. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/Updatable.java (line 31) <https://reviews.apache.org/r/46909/#comment196188> the other files I checked use LoggerFactory.getLogger -- is there a difference? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/Updatable.java (line 33) <https://reviews.apache.org/r/46909/#comment196189> this isn't called externally afaict. Why have it? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/Updatable.java (line 50) <https://reviews.apache.org/r/46909/#comment196181> aren't these units not matching? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/Updatable.java (line 80) <https://reviews.apache.org/r/46909/#comment196227> What if it fails? At some point you probably want to just start denying everything? because revokes would be completely broken. Maybe make that configurable? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/Updatable.java (line 89) <https://reviews.apache.org/r/46909/#comment196226> I'm not familiar with scheduleAtFixedRate, but do you actually need to sleep here? Can you just run the function and it will be called again at the fixed period? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java (line 30) <https://reviews.apache.org/r/46909/#comment196237> I would just collapse this and Updatable. It's not obvious the distinction will be useful going forward and if it is, we can always split it out later. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java (line 74) <https://reviews.apache.org/r/46909/#comment196223> just put this in the constructor sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java (line 86) <https://reviews.apache.org/r/46909/#comment196224> just put this in the constructor and make final sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java (line 95) <https://reviews.apache.org/r/46909/#comment196228> In the SentryConfigToolSolr we are using UserGroupInformation ugi = UserGroupInformation.getLoginUser(); String requestorName = ugi.getShortUserName(); any reason this is different? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java (line 117) <https://reviews.apache.org/r/46909/#comment196229> See above comment, you want to figure out how you want to handle errors, this isn't good enough. At a minimum, you need to write to the log. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java (line 123) <https://reviews.apache.org/r/46909/#comment196236> Is this what the generic backend is doing for listPrivilegesForProvider? That is pretty broken, enforcing a thrift->string mapping for privileges (i.e. listPrivilegesForProvider shouldn't even exist). Really I think the privilege mapping should be sentry-service-agnostic, and probably external-service specific. We already have privilege convertors...can we just use those? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java (line 145) <https://reviews.apache.org/r/46909/#comment196230> same here, figure out how you want to handle exceptions. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java (line 231) <https://reviews.apache.org/r/46909/#comment196179> This doesn't seem like the proper name for these constants. We are only supporting caching in the generic provider backend right now, right? So why do these have client in the name? Should probably be something like sentry.provider.backend.generic... I would probably also put them in the provider backend as well, but I see there are already provider backend constants in here, so do what you prefer in terms of placement. sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java (line 213) <https://reviews.apache.org/r/46909/#comment196177> use setBoolean sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java (line 214) <https://reviews.apache.org/r/46909/#comment196176> use setLong sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java (line 229) <https://reviews.apache.org/r/46909/#comment196178> probably want to sleep for like, 1.5 or 2 times this period. Documentation is: "Causes the currently executing thread to sleep (temporarily cease execution) for the specified number of milliseconds, subject to the precision and accuracy of system timers and schedulers"...sort of unclear if that means the cache will be updated in that time; but that's not a high precision thing we really care about, so I'd just be loose with it. Also, "sleepIfCachingEnabled" I would expect to...sleep if caching was enabled. This doesn't check if caching is enabled. - Gregory Chanan On May 6, 2016, 5:08 p.m., Ashish Singh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46909/ > ----------------------------------------------------------- > > (Updated May 6, 2016, 5:08 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/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/Updatable.java > PRE-CREATION > > 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 > 60502895a0fcdd781f5bd61b29a676a7c96f81b8 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java > dce3dade7f42fe35a849612c5caf2e98d2dac578 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > 00e3fbde76ebf84704ee110adfca30869845a7b8 > > 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 > >
