> On May 4, 2017, 12:52 p.m., kalyan kumar kalvagadda wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Lines 131 (patched) > > <https://reviews.apache.org/r/58221/diff/24/?file=1708098#file1708098line133> > > > > Can you update the comment explanining why this check was needed? I > > think you are using the HiveConf.ConfVars.METASTOREURIS.varname to make > > sure that the Hive server is already UP while running unit tests.
Yes. We want to prevent HMSFollower from using local meta store. > On May 4, 2017, 12:52 p.m., kalyan kumar kalvagadda wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java > > Lines 182 (patched) > > <https://reviews.apache.org/r/58221/diff/24/?file=1708100#file1708100line182> > > > > As part of shutdownAndAwaitTermination, shutdown is called first to > > stop new tasks to getting scheduled. > > > > Later awaitTermination is called with timeout value while actually the > > interval at which the tasks are scheduled giving it enough time to > > terminate before calling shutdownNow. > > > > When awaitTermination is called for the second time to log an error, i > > feel the timeout can be 0, as we need not give any more time for the task > > to temrinate. If HMSFollower is getting full snapshot, it may take longer. So wait for additional time is useful. > On May 4, 2017, 12:52 p.m., kalyan kumar kalvagadda wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > > Line 172 (original), 172 (patched) > > <https://reviews.apache.org/r/58221/diff/24/?file=1708101#file1708101line172> > > > > Understand that this delay is causing issues during tests but making it > > 0 may not be a best choice. > > > > It may take some time for the server to initialize the database and be > > completely up to handle the notifications. > > > > If 60000 milli sec is higher value, we should have some smaller value > > instead. since we prevent connecting to local meta store, HMSFollower does nothing if Hive meta store is not up. It does not hurt to have this value being zero, and allow sentry getting updates sooner. - Na ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58221/#review173895 ----------------------------------------------------------- On May 5, 2017, 3:47 p.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58221/ > ----------------------------------------------------------- > > (Updated May 5, 2017, 3:47 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 > 99549bc > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 132db63 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java > ce73358 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > 834ed41 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java > 2ebe561 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java > 212c465 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java > b5247d0 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java > 1ace07c > > > Diff: https://reviews.apache.org/r/58221/diff/25/ > > > Testing > ------- > > > Thanks, > > Na Li > >
