> On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote: > > There are many unrelated formatting changes which should be removed.
I will fix it in next update > On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > > Lines 107 (patched) > > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line107> > > > > Why do you need the future for the cleaner? I explained in the same question above. This is duplicate. > On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote: > > 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/diff/9/?file=1692270#file1692270line198> > > > > Can you clarify why it should be started in constructor and why would > > tests fail otherwise? the test TestDbSentryOnFailureHookLoading setup calls AbstractTestWithDbProvider.createContext(), which expects the context to be not null after creating SentryService and before calling SentryService.start(). If we don't schedule the cleaner in constructor, we will fail at assumeNotNull(context); if we schedul the cleaner in constructor, it does not fail at assumeNotNull(context); AbstractTestWithDbProvider.createContext() { ... assumeNotNull(context); context = AbstractTestWithHiveServer.createContext(properties); policyFile .setUserGroupMapping(StaticUserGroup.getStaticMapping()) .write(context.getPolicyFile(), policyFilePath); startSentryService(); ... } > On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > > Lines 221 (patched) > > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line249> > > > > Why do we need to start it again if it is already started in > > constructor? In test, if stop() is called and then start() is called, not calling startSentryStoreCleaner() in runServer() would fail the test > On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > > Lines 290 (patched) > > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line318> > > > > 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. I will change it to IllegalArgumentException. It is the only possible exception > On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > > Lines 298 (patched) > > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line326> > > > > 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. will update > On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > > Lines 300 (patched) > > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line328> > > > > 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 I will wrap shutdownNode in catch block with try-catch > On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > > Lines 305 (patched) > > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line333> > > > > 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. I will add a comment > On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > > Lines 327 (patched) > > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line355> > > > > It would be nice to log the fact that we started the service (if we did) will add log > On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > > Lines 330 (patched) > > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line358> > > > > 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. I do need to check future. Will update accordingly to style > On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > > Lines 333 (patched) > > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line361> > > > > Do we need to start an executor if it isn't going to be used due to > > SENTRY_STORE_CLEAN_PERIOD_SECONDS settings? I will start the executor only when it is needed > On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > > Lines 356 (patched) > > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line384> > > > > Exception is too broad here will change it to IllegalArgumentException > On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > > Lines 364 (patched) > > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line392> > > > > Same comment - it is usually better to have > > > > if (sentryStoreCleanerService == null) { > > return; > > } will update > On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > > Lines 367 (patched) > > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line395> > > > > shutdownNow() should be good enough here. This is original code I moved from constructor. There may be some reason I don't know that the shut down is so complicated. > On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > > Lines 368 (patched) > > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line396> > > > > 10 seconds seems a bit excessive This is original valued. What value you suggest? > On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > > Lines 370 (patched) > > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line398> > > > > Do we need to call awaitTermination() after shutdownNow()? This is > > existing code, but still. I don't know the reason why > On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > > Lines 374 (patched) > > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line402> > > > > catch parameter ie is unused I will add log > On April 14, 2017, 12:57 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > > Lines 425 (patched) > > <https://reviews.apache.org/r/58221/diff/9/?file=1692270#file1692270line454> > > > > 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. the synchronized is orignal. You mentioned if multiple threads accese it at the same time, since we don't protect variables, it is still not thread safe. - Na ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58221/#review171970 ----------------------------------------------------------- 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 > >
