----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56720/#review165907 -----------------------------------------------------------
I would consider refactoring this a bit. IMO it is better to have SentryStore as just a DB access layer class and have services like this cleaner to be created outside of SentryStore by consumers so whoever sets up SentryStore should decide whether they want to have a cleaner running as well (and we need to make sure that we do not run multiple cleaners). Some tests for example, may create multiple SentryStore instances. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 230) <https://reviews.apache.org/r/56720/#comment237741> it should use leaderMonitor.isLeader() method instead. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (line 89) <https://reviews.apache.org/r/56720/#comment237740> That's wrong - isLeader is a snapshot of the state. You need to pass actual leaderMonitor. Also, it is weird that we are creating SentryStore in HMSFollower - something is wrong here. - Alexander Kolbasov On Feb. 16, 2017, 11:37 p.m., Lei Xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56720/ > ----------------------------------------------------------- > > (Updated Feb. 16, 2017, 11:37 p.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/persistent/SentryStore.java > 4cc123fd0de05a055b4276d213eafc0eb070213d > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 783c0fcf5e338897b56c4e17ba1669394c02e8a3 > > 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 > >
