> On Feb. 11, 2015, 9:04 p.m., Lenni Kuff wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/ha/TestHaEnd2End.java, > > line 77 > > <https://reviews.apache.org/r/30839/diff/2/?file=860514#file860514line77> > > > > Add one negative test case for privileges on an object.
It covers negative cases like all verifying errors with all services down. > On Feb. 11, 2015, 9:04 p.m., Lenni Kuff wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java, > > line 44 > > <https://reviews.apache.org/r/30839/diff/2/?file=860516#file860516line44> > > > > 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. Added comments and error for numServers <= 0. > On Feb. 11, 2015, 9:04 p.m., Lenni Kuff wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/SentryServFactory.java, > > line 26 > > <https://reviews.apache.org/r/30839/diff/2/?file=860517#file860517line26> > > > > Since we don't support external perhaps just leave this out for now, > > but mention how we might use it in the future? A dummy external server implementation will be in a followup ticket which should include the details. will file the jira shortly. > On Feb. 11, 2015, 9:04 p.m., Lenni Kuff wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/SentrySrv.java, > > line 34 > > <https://reviews.apache.org/r/30839/diff/2/?file=860518#file860518line34> > > > > Should this be part of the interface? Seems like it is only applicable > > in the HA case. It should be part of the interface. The clients would need the ZK connection string in order to connect to Sentry service. > On Feb. 11, 2015, 9:04 p.m., Lenni Kuff wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/SentrySrv.java, > > line 36 > > <https://reviews.apache.org/r/30839/diff/2/?file=860518#file860518line36> > > > > what's the difference between stopAll and shutdown? Shutdonw closes the ZK along with all the running servers. The SentrySrv is not usable after shutdown. Added the comment on the interface describing the difference. Also added a check to validate the state for all APIs. On Feb. 11, 2015, 9:04 p.m., Prasad Mujumdar wrote: > > This is very cool. Would it also be possible to allow running all the > > existing tests with HA enabled (no failover)? Added a property sentry.e2etest.enable.service.ha that can be set to true via maven commandline to run tests with HA. Since the patch changed the framework to use the new SentrySrv abstraction, this can be used to run any tests with HA. Just that it slows donw the test runs and currently not enabled by default. - Prasad ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30839/#review72018 ----------------------------------------------------------- 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 > >
