----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58221/#review171970 -----------------------------------------------------------
There are many unrelated formatting changes which should be removed. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Lines 107 (patched) <https://reviews.apache.org/r/58221/#comment244985> Why do you need the future for the cleaner? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Line 196 (original), 186 (patched) <https://reviews.apache.org/r/58221/#comment244986> Can you clarify why it should be started in constructor and why would tests fail otherwise? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Lines 221 (patched) <https://reviews.apache.org/r/58221/#comment244987> Why do we need to start it again if it is already started in constructor? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Lines 290 (patched) <https://reviews.apache.org/r/58221/#comment244988> Exception is too broad here - you know what are possible exceptions. In the older code you were also handling exceptions from HMSFollower construction - now you are not. It is impossible to handle the failure so the best thing to do here is to propagate the exception. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Lines 298 (patched) <https://reviews.apache.org/r/58221/#comment244989> Style. In such cases it is better to use if (hmsFollowerExecutor == null) { return; // nothing to stop } This way you don't need to have long chain of conditions and follow the else clauses. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Lines 300 (patched) <https://reviews.apache.org/r/58221/#comment244991> Your try { } block is too broad. Are you trying shutdown() or shutdownNow()? Also you run shutdownNow inside a try block but then call it in the catch anyway, so it isn't very clear what are you guarding. It may be better to surround specific calls with try blocks so that you can handle individual failures properly sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Lines 305 (patched) <https://reviews.apache.org/r/58221/#comment244990> Why are you using SENTRY_HMSFOLLOWER_INTERVAL_MILLS as a timeout value here? Not that it is particularly wrong, but at least it is worth a comment. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Lines 327 (patched) <https://reviews.apache.org/r/58221/#comment245002> It would be nice to log the fact that we started the service (if we did) sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Lines 330 (patched) <https://reviews.apache.org/r/58221/#comment244995> I don't think you need a future here, but even if you do it is better to check it outside the try block and return immediately if it is not null. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Lines 333 (patched) <https://reviews.apache.org/r/58221/#comment244992> Do we need to start an executor if it isn't going to be used due to SENTRY_STORE_CLEAN_PERIOD_SECONDS settings? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Lines 356 (patched) <https://reviews.apache.org/r/58221/#comment244993> Exception is too broad here sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Lines 364 (patched) <https://reviews.apache.org/r/58221/#comment244994> Same comment - it is usually better to have if (sentryStoreCleanerService == null) { return; } sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Lines 367 (patched) <https://reviews.apache.org/r/58221/#comment244996> shutdownNow() should be good enough here. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Lines 368 (patched) <https://reviews.apache.org/r/58221/#comment244997> 10 seconds seems a bit excessive sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Lines 370 (patched) <https://reviews.apache.org/r/58221/#comment244998> Do we need to call awaitTermination() after shutdownNow()? This is existing code, but still. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Lines 374 (patched) <https://reviews.apache.org/r/58221/#comment245001> catch parameter ie is unused sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Lines 425 (patched) <https://reviews.apache.org/r/58221/#comment245000> The code uses synchronized() on both, so the comment isn't quite right - it would handle concurrent calls to start()/stop() - not that it is needed. - Alexander Kolbasov On April 13, 2017, 7:56 p.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58221/ > ----------------------------------------------------------- > > (Updated April 13, 2017, 7:56 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/9/ > > > Testing > ------- > > > Thanks, > > Na Li > >
