You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Vamsee Yarlagadda <va...@cloudera.com> on 2017/06/06 00:06:55 UTC
Re: Review Request 58727: SENTRY-1708 Extend the current test classes
to handle multiple sentry servers
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58727/#review176967
-----------------------------------------------------------
First pass comments. Will take a deeper look.
sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/SentryHdfsServiceIntegrationBase.java
Lines 56-62 (original), 56-59 (patched)
<https://reviews.apache.org/r/58727/#comment250506>
Can't we set the value to pass all the live sentry servers like we do on real clusters?
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java
Lines 141-148 (patched)
<https://reviews.apache.org/r/58727/#comment250507>
Do we handle to cleanup as part of create method?
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java
Lines 150 (patched)
<https://reviews.apache.org/r/58727/#comment250508>
I would prefer to see this being set by the tests itself whether they want a single server or multi server.
I was thinking for the fact that we have a good set of unit tests coverage, one could simply parameterize the tests to run the same sceanrio in both single server and multi server mode to get lot of coverage for free.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java
Lines 256-258 (original), 382-388 (patched)
<https://reviews.apache.org/r/58727/#comment250509>
shutdownAllSentryServices() ?
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java
Lines 119-123 (original), 113-117 (patched)
<https://reviews.apache.org/r/58727/#comment250510>
shutdownAllSentryServices()?
- Vamsee Yarlagadda
On May 12, 2017, 3:49 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58727/
> -----------------------------------------------------------
>
> (Updated May 12, 2017, 3:49 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1708
> https://issues.apache.org/jira/browse/SENTRY-1708
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Here is the abstract of the code changes done. There are basically changes to the base classes used tests to be able to handle multiple servers
> List of sentry sentry servers
> List of the address used for these servers
> This is learnt so re-use the same port when the servers are re-started
> API's to start/stop all the servers
> API's to start/stop specific servers
> API to lean the server addresses
> API to update the sentry server config list for the clients to use
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/SentryHdfsServiceIntegrationBase.java eccf83b
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericServiceClient.java 8959ad8
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryPolicyServiceClient.java 3b3b30e
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceClientPool.java fe4164d
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceFailureCase.java 7c7ebab
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceMetrics.java a33b03a
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceWithInvalidMsgSize.java a4dd8a6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java dd37e7e
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java 17a2d1e
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestPrivilegeWithHAGrantOption.java 53cbd00
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java e2fb36a
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java 054b193
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/SentrySrv.java dac1151
> sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java bead003
> sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/AbstractSqoopSentryTestBase.java 8a01e1c
>
>
> Diff: https://reviews.apache.org/r/58727/diff/3/
>
>
> Testing
> -------
>
> Made sure that all the db-provider unit tests passed.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>