> On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Lines 108 (patched) > > <https://reviews.apache.org/r/58221/diff/5/?file=1689529#file1689529line114> > > > > No need to return at the end of the function.
will remove > On April 12, 2017, 4:50 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Line 376 (original), 383 (patched) > > <https://reviews.apache.org/r/58221/diff/5/?file=1689529#file1689529line389> > > > > As you are fixing this, can you fix this line as well - we know that > > dbNam == null so we shouldn't try to convert it to string. will update > 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 77 (patched) > > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line77> > > > > Style - it is better to have field declarations first, followed by > > methods. The close() method is usually somewhere closer to the end but we > > don't have any specific policy on that. I will move it to the end of the file > 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? It is possible. I will add "synchronized" in front of it. > 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 81 (patched) > > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line81> > > > > Note that you need to stop the executor first. Otherwise while you are > > calling close() it may run HMSFOllower which isn't good. stop() should be called before this, so the task is already cancelled. therefore, the executor won't schedule the task again. To be safe, I will change the order. > 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 92 (patched) > > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line92> > > > > Can you call awaitTermination() before you called shutdown()? > > > > The doc says: > > > > Blocks until all tasks have completed execution after a shutdown > > request, or the timeout occurs, or the current thread is interrupted, > > whichever happens first. I believe we should call shutdown() first. So no new tasks can be scheduled > 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. >From API, it could be thrown > 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 99 (patched) > > <https://reviews.apache.org/r/58221/diff/5/?file=1689530#file1689530line99> > > > > Should we call shutdownNow() in this case? Yes. I will add it > 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? to track the task that schedules hmsFollower, so I can cancel it in stop() > 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? to track the task that schedules sentryStoreCleaner, so I can cancel it in stop() > 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? 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. - Na ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58221/#review171666 ----------------------------------------------------------- On April 12, 2017, 3:47 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, 3:47 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 > 6c14f5e > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 132db63 > > > Diff: https://reviews.apache.org/r/58221/diff/5/ > > > Testing > ------- > > > Thanks, > > Na Li > >
