----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46909/#review131419 -----------------------------------------------------------
Note: these comments are based on the assumption you are implementing this at the client layer. See my previous comments for why I think it's a better idea right now to do this at the ProviderBackend layer. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java (line 67) <https://reviews.apache.org/r/46909/#comment195386> make these final? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java (line 168) <https://reviews.apache.org/r/46909/#comment195385> You may want to reserve some configuration space for future cache implementations. For example, you could define a name for this implementation, e.g. "simpleTTL" and include this name in each configuration value, e.g. this could be ".simpleTTL.TTLMS" or whatever. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java (line 442) <https://reviews.apache.org/r/46909/#comment195389> This means that if I ask for a group that doesn't exist, I have to go to the server to verify that, right? Even though presumably I could have everything in the cache and just reject this immediately and make the user wait for the cache to be updated. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java (line 443) <https://reviews.apache.org/r/46909/#comment195391> it's generally a good idea to put {} around these, it's easy to add another line of impl to the if and have it always execute by mistake. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java (line 457) <https://reviews.apache.org/r/46909/#comment195392> see above about {}. also, does this actually work? Don't you want the key to be by group name and component? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java (line 491) <https://reviews.apache.org/r/46909/#comment195393> see above. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java (line 668) <https://reviews.apache.org/r/46909/#comment195399> Use nanoTime, see here for more information: http://memorynotfound.com/calculating-elapsed-time-java/ (you can still take millis as the configuration). - Gregory Chanan On May 2, 2016, 9:32 p.m., Ashish Singh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46909/ > ----------------------------------------------------------- > > (Updated May 2, 2016, 9:32 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 SentryGenericServiceClientDefaultImpl. > > > Diffs > ----- > > > 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-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericServiceIntegration.java > fcf0e7b9dc45eb12cc1b76f3084efd6340c039a4 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericServiceIntegrationWithCaching.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/46909/diff/ > > > Testing > ------- > > Extended unit tests. Will be running perf tests for Kafka-Sentry integration > with caching turned on. > > > Thanks, > > Ashish Singh > >
