----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58727/#review174254 -----------------------------------------------------------
sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java Lines 25 (patched) <https://reviews.apache.org/r/58727/#comment247384> Please remove unused imports, don't just comment them sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java Line 65 (original), 74 (patched) <https://reviews.apache.org/r/58727/#comment247388> Is it final or not? Do we always start two servers? The actual patch has this set to 1 - which one is correct? sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java Lines 75 (patched) <https://reviews.apache.org/r/58727/#comment247385> should this be final? sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java Lines 76 (patched) <https://reviews.apache.org/r/58727/#comment247386> Should this be final? sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java Lines 120 (patched) <https://reviews.apache.org/r/58727/#comment247387> This seems to be unused sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java Lines 138 (patched) <https://reviews.apache.org/r/58727/#comment247389> Why would servers be ever null? sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java Lines 142 (patched) <https://reviews.apache.org/r/58727/#comment247391> This essentially means: boolean reuseCurrentConfig = !serverAddresses.isEmpty(); sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java Lines 144 (patched) <https://reviews.apache.org/r/58727/#comment247392> We already have servers allocated - why allocate again? sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java Lines 149 (patched) <https://reviews.apache.org/r/58727/#comment247393> Note that even if you reuse existing configs, the new server may be started on a different port. - Alexander Kolbasov On May 5, 2017, 6:52 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58727/ > ----------------------------------------------------------- > > (Updated May 5, 2017, 6:52 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 > dfd79ae > > 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/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/2/ > > > Testing > ------- > > Made sure that all the db-provider unit tests passed. > > > Thanks, > > kalyan kumar kalvagadda > >
