> 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?
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. > 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. Will do. > 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. I will discuss with you tomorrow at hipchat to see if I will change back to 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 12, 2017, 5:38 a.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58221/ > ----------------------------------------------------------- > > (Updated April 12, 2017, 5:38 a.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/6/ > > > Testing > ------- > > > Thanks, > > Na Li > >
