aahmed-se commented on issue #2101: [WIP] Migrate compaction test to testcontainers URL: https://github.com/apache/incubator-pulsar/pull/2101#issuecomment-403423400 Almost done with the migration gotten rid of the arquillian dependencies tests are not passing yet so have to debug a bit. -Ali On Mon, Jul 9, 2018 at 2:38 AM Ivan Kelly <[email protected]> wrote: > *@ivankelly* requested changes on this pull request. > > Compaction and s3 should be changed in separate PRs. As it is I don't > think s3 is being migrated. > ------------------------------ > > In > tests/integration/s3-offload/src/test/java/org/apache/pulsar/tests/integration/TestS3Offload.java > <https://github.com/apache/incubator-pulsar/pull/2101#discussion_r200938995> > : > > > @@ -59,9 +61,11 @@ > @BeforeMethod > public void configureAndStartBrokers() throws Exception { > > - String s3ip = DockerUtils.cubeIdsWithLabels( > - docker, ImmutableMap.of("service", "s3", "cluster", CLUSTER_NAME)) > - .stream().map((c) -> DockerUtils.getContainerIP(docker, c)).findFirst().get(); > +// String s3ip = DockerUtils.cubeIdsWithLabels( > > Remove commented out code. > ------------------------------ > > In > tests/integration-tests-topologies/src/main/java/org/apache/pulsar/tests/topologies/PulsarCluster.java > <https://github.com/apache/incubator-pulsar/pull/2101#discussion_r200939801> > : > > > @@ -112,6 +108,12 @@ private PulsarCluster(PulsarClusterSpec spec) { > .withEnv("zookeeperServers", ZKContainer.NAME) > .withEnv("configurationStoreServers", CSContainer.NAME + ":" + CS_PORT) > .withEnv("clusterName", clusterName); > + > + this.s3Container = new S3Container(clusterName, S3Container.NAME) > > It'd be better to create a subclass of PulsarCluster, PulsarClusterWithS3, > and only start S3 in that case. As it is now, S3 is being started for all > cases. > ------------------------------ > > In > tests/integration/s3-offload/src/test/java/org/apache/pulsar/tests/integration/TestS3Offload.java > <https://github.com/apache/incubator-pulsar/pull/2101#discussion_r200940135> > : > > > @@ -44,6 +45,7 @@ > import org.testng.Assert; > import org.testng.annotations.AfterMethod; > import org.testng.annotations.BeforeMethod; > +import org.testng.annotations.DataProvider; > import org.testng.annotations.Test; > > public class TestS3Offload extends Arquillian { > > Still using Arquillian here? > > — > You are receiving this because you were assigned. > Reply to this email directly, view it on GitHub > <https://github.com/apache/incubator-pulsar/pull/2101#pullrequestreview-135342126>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AHo1fSXGvfzdGrkVaArD1t1YgcjYmqtMks5uEySKgaJpZM4VEyUu> > . > -- -Ali
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
