----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30839/#review72018 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/ServiceManager.java <https://reviews.apache.org/r/30839/#comment117956> While you are here can you add a brief comment to this class? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HAClientInvocationHandler.java <https://reviews.apache.org/r/30839/#comment117943> perhaps add this in a "finally" block so we ensure it gets closed even in the event of an error? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java <https://reviews.apache.org/r/30839/#comment117944> rename "future" to something more descriptive of what it actually is. sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/ha/TestHaEnd2End.java <https://reviews.apache.org/r/30839/#comment117945> Comment on what this test suite is covering. sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/ha/TestHaEnd2End.java <https://reviews.apache.org/r/30839/#comment117946> Add one negative test case for privileges on an object. sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java <https://reviews.apache.org/r/30839/#comment117947> nit getSentrySrv sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java <https://reviews.apache.org/r/30839/#comment117954> comment - only set if HA is enabled (numServers>1). sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java <https://reviews.apache.org/r/30839/#comment117953> it's not immediately obvious that setting > 1 servers automatically enables HA. Mention this behavior. It might be more clear if you have two params: "bool enableHa, int numServers" and maybe throw an error if enableHa=false && numServers > 1. Also throw an IllegalArgumentException if numServers <= 0. sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/SentryServFactory.java <https://reviews.apache.org/r/30839/#comment117951> Consistent naming - sentrySrv or sentryServ or sentryService. all of them are fine, but pick one and use it consistently. sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/SentryServFactory.java <https://reviews.apache.org/r/30839/#comment117952> Since we don't support external perhaps just leave this out for now, but mention how we might use it in the future? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/SentrySrv.java <https://reviews.apache.org/r/30839/#comment117948> Comment on public interface and all public methods. Clients should know how to implement these. sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/SentrySrv.java <https://reviews.apache.org/r/30839/#comment117949> Should this be part of the interface? Seems like it is only applicable in the HA case. sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/SentrySrv.java <https://reviews.apache.org/r/30839/#comment117955> what's the difference between stopAll and shutdown? This is very cool. Would it also be possible to allow running all the existing tests with HA enabled (no failover)? - Lenni Kuff On Feb. 10, 2015, 9:22 p.m., Prasad Mujumdar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30839/ > ----------------------------------------------------------- > > (Updated Feb. 10, 2015, 9:22 p.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 > >
