> 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 78 (patched) > > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line78> > > > > Do you ever expect concurrent calls to close() or not? > > Na Li wrote: > It is possible. I will add "synchronized" in front of it. > > Alexander Kolbasov wrote: > Under what circumstances is it possible? If it is possible, it may not be > sufficient to add synchronized() - you may need to protect access to the > variables as well, but I a not convinced that it is possible. Do we run our > tests in parallel? > > Na Li wrote: > I am not familar with how Sentry service is actually used. > Hypathetically, it is possible that several threads hold the same instance > and call its close() concurrently. > > Alexander Kolbasov wrote: > This should never hapen during regular use - I think this may only happen > if tests are running in parallel but I guess that it will break many things.
close() is removed. So this issue does not apply any more. > 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 96 (patched) > > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line96> > > > > I think this should never happen. > > Na Li wrote: > From API, it could be thrown > > Alexander Kolbasov wrote: > from the API - yes, but this shouldn't happen in our case, so we should > log something about the fact that this should never happen. > > Na Li wrote: > Will do. add the message in log > 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 145 (patched) > > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line145> > > > > Why do you need this? > > Na Li wrote: > to track the task that schedules hmsFollower, so I can cancel it in stop() it is removed. > 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() 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 > 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 350 (patched) > > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line380> > > > > This is most likely wrong. What does the cancel of the future do? What > > task does it actually cancel given that this is periodic executor? U think > > you should just use shutdown on the executor instead of this. > > Alexander Kolbasov wrote: > It seems to be not exactly wrong, but makes things more complicated then > they should be. You need to shutdown the executor on exit anyway, so why > cancel scheduling using futures? > > Na Li wrote: > Based on java doc and discussion in > http://stackoverflow.com/questions/33022402/stopping-and-removing-a-task-from-scheduledexecutorservice, > it seems future.cancel can stop the task from being scheduled in the future. > I can run the testing code and verify the behavior of Future.cancel() > tomorrow. If this is the case, then cancel future is less overhead then > shutting down the ExecutorService. Once ExecutorService is shutdown, it is > useless, and we have to create a new ExecutorService in order to use it. > > Alexander Kolbasov wrote: > Cancel is useful if we want to stop execution but want to keep the > executor for later use. Normally (non-test use) we do want to cancel the > executor itself - we never need to reuse the executor. For tests we can keep > it, but then we need to have test-only code, and the overhead isn't important. > > Na Li wrote: > I will discuss with you tomorrow at hipchat to see if I will change back > to shutdown the executor at SentryService.stop() I have changed the code and shutdown the executor at SentryService.stop() - Na ----------------------------------------------------------- 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 > >
