----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56720/#review166015 -----------------------------------------------------------
Fix it, then Ship it! Ship It! sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryChange.java (line 23) <https://reviews.apache.org/r/56720/#comment237901> Nit - since this is empty, should we use empty interface instead? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 428) <https://reviews.apache.org/r/56720/#comment237902> This seems to be test-only function to cleanup database between tests, so most likely this should stay. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 459) <https://reviews.apache.org/r/56720/#comment237905> This is a neat way to do it. Can we parameterize this by number of deltas to keep (if e.g. for debugging we'll decide to keep a bunch)? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 480) <https://reviews.apache.org/r/56720/#comment237903> It would be nice to know what of the two calls caused exception - may be it is better to trap it inside the purgeDeltaChangeTableCore() then we can log class name that caused trouble. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 3406) <https://reviews.apache.org/r/56720/#comment237906> Since we know the return type, it may be better to specify List<T> here and cast the result of execute. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java (line 124) <https://reviews.apache.org/r/56720/#comment237907> Who is consuming this? - Alexander Kolbasov On Feb. 18, 2017, 1:26 a.m., Lei Xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56720/ > ----------------------------------------------------------- > > (Updated Feb. 18, 2017, 1:26 a.m.) > > > Review request for sentry, Alexander Kolbasov and Hao Hao. > > > Bugs: SENTRY-1611 > https://issues.apache.org/jira/browse/SENTRY-1611 > > > Repository: sentry > > > Description > ------- > > Provide a background service to periodically purge path and permission delta > change tables. > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryChange.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java > 952b11f4e81426ec51c0ad6131ad579ba5e3383e > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java > 3662bfdebd1a4e2172ab83bd8c8e60ccf21b55b9 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 4cc123fd0de05a055b4276d213eafc0eb070213d > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > e10ddfd0372c985e5a8bd6d5b3ca07d5cb618a68 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > dfaac1555fe729efab3f8fdccb914978001fee56 > > Diff: https://reviews.apache.org/r/56720/diff/ > > > Testing > ------- > > Run `mvn test -Dtest=TestSentryStore` > > > Thanks, > > Lei Xu > >
