----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30839/#review71944 -----------------------------------------------------------
Ship it! Hi Prasad, most of the patch looks good to me, some minor comments below. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HAClientInvocationHandler.java <https://reviews.apache.org/r/30839/#comment117860> Hi Prasad, the change will create a curatorFramework for each renew operation, I think a close method should be added at InvocationHandler, I added a close method to HAClientInvocationHandler in the connection pool jira, but patch is under reviewing by Lenni, so I'm agreed with the change currently sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java <https://reviews.apache.org/r/30839/#comment117864> It seems here is 180s sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java <https://reviews.apache.org/r/30839/#comment118075> need we catch the exception of stop? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java <https://reviews.apache.org/r/30839/#comment118074> If the first sentry server got an exception when do stopping, other servers may never execute stop() sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java <https://reviews.apache.org/r/30839/#comment118064> Nit: shutdown() may be a little ambiguous, how about clean() or close() - Dapeng Sun On 二月 11, 2015, 5:22 a.m., Prasad Mujumdar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30839/ > ----------------------------------------------------------- > > (Updated 二月 11, 2015, 5:22 a.m.) > > > Review request for sentry and Dapeng Sun. > > > Bugs: SENTRY-648 > https://issues.apache.org/jira/browse/SENTRY-648 > > > Repository: sentry > > > Description > ------- > > - Update test framework to allow creating multiple services > - Sentry service abstraction to enable start/stop of individual servers from > test > - Added test cases > - Fixed issues ZK session leak found with the new tests > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/ServiceManager.java > 2274f00 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HAClientInvocationHandler.java > c6e265f > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 02788aa > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > ddc5930 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/ha/TestHaEnd2End.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java > d08b4ee > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/SentryServFactory.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/SentrySrv.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/30839/diff/ > > > Testing > ------- > > Added New tests - > - Normal Sentry operations with HA enabled > - Testing service failover > > > Thanks, > > Prasad Mujumdar > >
