> On May 9, 2017, 12:35 a.m., Alexander Kolbasov wrote: > > 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/diff/2/?file=1709540#file1709540line25> > > > > Please remove unused imports, don't just comment them
I missed some of them. Corrected the rest. > On May 9, 2017, 12:35 a.m., Alexander Kolbasov wrote: > > 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/diff/2/?file=1709540#file1709540line74> > > > > Is it final or not? Do we always start two servers? > > The actual patch has this set to 1 - which one is correct? It should still be 1. Based on the test appropriate classes would overide this value. My recent patches have test failure upstream so I have not updated the review board. > On May 9, 2017, 12:35 a.m., Alexander Kolbasov wrote: > > 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/diff/2/?file=1709540#file1709540line75> > > > > should this be final? No it should not. They should be initialized later based on the sentryServerCount. This could updated by the child classes. > On May 9, 2017, 12:35 a.m., Alexander Kolbasov wrote: > > 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/diff/2/?file=1709540#file1709540line76> > > > > Should this be final? No it should not. They should be initialized later based on the sentryServerCount. This could updated by the child classes. > On May 9, 2017, 12:35 a.m., Alexander Kolbasov wrote: > > 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/diff/2/?file=1709540#file1709540line120> > > > > This seems to be unused This change set provides the API's needed for the failover tests to use. New tests for failover will use it. > On May 9, 2017, 12:35 a.m., Alexander Kolbasov wrote: > > 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/diff/2/?file=1709540#file1709540line138> > > > > Why would servers be ever null? When the all the servers are shutdown, servers is made null. > On May 9, 2017, 12:35 a.m., Alexander Kolbasov wrote: > > 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/diff/2/?file=1709540#file1709540line142> > > > > This essentially means: > > > > boolean reuseCurrentConfig = !serverAddresses.isEmpty(); Yes, at the beginning of the method only. > On May 9, 2017, 12:35 a.m., Alexander Kolbasov wrote: > > 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/diff/2/?file=1709540#file1709540line144> > > > > We already have servers allocated - why allocate again? I have initialized it null now. Based on sentryServerCount value servers and serverAddresses are initialized. Test classes for HA will update the sentryServerCount so that they have multiple sentry servers. > On May 9, 2017, 12:35 a.m., Alexander Kolbasov wrote: > > 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/diff/2/?file=1709540#file1709540line149> > > > > Note that even if you reuse existing configs, the new server may be > > started on a different port. That is what we want to avoid. we store the serverAddresses seperately so that they can be re-used when the server is restarted. When the server is restarted, we take port number from corresponding serverAddresses and update the configuration with the same port. It is not guarenteed that it works but the chances of the port that is been released to get assigned is pretty low. Why do you think server may be started on a different port? - kalyan kumar ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58727/#review174254 ----------------------------------------------------------- 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 > >
