----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66792/#review202073 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 684-687 (patched) <https://reviews.apache.org/r/66792/#comment283732> I have two comments here 1. This delete should be straggered. There could be huge data speread accross multiple snapshots. This is take much longer. If there are too manu snpshots we should limit on how many shots can be purged at one run. Let's say there are 20 snapshots already existing, we could have a behaviour where each time the purge is scheduled it cleans-up 5 snaphots. 2. we should find a way to delete entires in AUTHZ_PATH and AUTHZ_PATHS_MAPPING togerther in single datanuclesu call. that will be efficient. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 690-692 (patched) <https://reviews.apache.org/r/66792/#comment283727> This is not efficient as you are stoing all the paths objects in memory before trying to delete them from the database. There could be millions of path entires. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 705 (patched) <https://reviews.apache.org/r/66792/#comment283729> Provoding a huge list of patyh entires might be inefficient as datanucleus is going to perform a delete from each entiry sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Lines 382-383 (patched) <https://reviews.apache.org/r/66792/#comment283726> Isn't it better to have both of these in single transaction? - kalyan kumar kalvagadda On April 27, 2018, 3:57 a.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66792/ > ----------------------------------------------------------- > > (Updated April 27, 2018, 3:57 a.m.) > > > Review request for sentry, Alexander Kolbasov, Arjun Mishra, kalyan kumar > kalvagadda, and Sergio Pena. > > > Repository: sentry > > > Description > ------- > > Add function to periodically remove the old path snapshot > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 8ac3c0d > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 4236c02 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > a66d91e > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > b410027 > > > Diff: https://reviews.apache.org/r/66792/diff/2/ > > > Testing > ------- > > unit tests in TestSentryStore. > > > Thanks, > > Na Li > >