> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > > Lines 160 (patched) > > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line160> > > > > Why do you need this? > > Na Li wrote: > to track the task that schedules sentryStoreCleaner, so I can cancel it > in stop() > > Na Li wrote: > I still keep it to mark that the cleaner is scheduled if the future is > not null. > > The problems are > 1. We need to create sentryStoreCleanService and schedule cleaner (call > startSentryStoreCleaner()) in runServer() if SentryService.start() is called > after SentryService.stop(), which shutdown sentryStoreCleanService. > Otherwise, the cleaner cannot be scheduled any more. Test may fail. > 2. We need to create sentryStoreCleanService and schedule cleaner in > SentryService constructor (call startSentryStoreCleaner()), so the test > TestDbSentryOnFailureHookLoading won't fail. It expects the context to be not > null in createContext() before calling SentryService.start(). > 3. To satify item 1. and 2., we need to call startSentryStoreCleaner() at > both constructor and runServer(). To avoid scheduling the cleaner twice, we > need to detect if it is scheduled already. > 4. When I tried to fix test failures, it seems it is better to create the > sentryStoreCleanService at the beginning of creating SentryService. Then > checking if sentryStoreCleanService is null or not cannot detect if the > cleaner is scheduled already. > > > When the SentryService is created, we want to create > sentryStoreCleanService before calling startSentryStoreCleaner(). But after > stop(), we need to create sentryStoreCleanService within > startSentryStoreCleaner. So I choose to use sentryStoreCleanerFuture to > decide if we should schedule the cleaner
Thanks for the explanation. This looks like complete mess. You don't have to untangle it in this JIRA, but please 1) Add a comment with the above explanation 2) File a follow-up JIRA for cleaning this mess up. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58221/#review171666 ----------------------------------------------------------- On April 14, 2017, 8:36 p.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58221/ > ----------------------------------------------------------- > > (Updated April 14, 2017, 8:36 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar > kalvagadda, and Sergio Pena. > > > Repository: sentry > > > Description > ------- > > SENTRY-1649 move HMS follower to runServer > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 16676fb > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 132db63 > > > Diff: https://reviews.apache.org/r/58221/diff/11/ > > > Testing > ------- > > > Thanks, > > Na Li > >
