You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2022/04/18 06:31:46 UTC

[GitHub] [druid] kfaraz commented on a diff in pull request #12368: First cut at restructuring the integration tests

kfaraz commented on code in PR #12368:
URL: https://github.com/apache/druid/pull/12368#discussion_r851582822


##########
docker-tests/README.md:
##########
@@ -0,0 +1,102 @@
+<!--
+  ~ Licensed to the Apache Software Foundation (ASF) under one
+  ~ or more contributor license agreements.  See the NOTICE file
+  ~ distributed with this work for additional information
+  ~ regarding copyright ownership.  The ASF licenses this file
+  ~ to you under the Apache License, Version 2.0 (the
+  ~ "License"); you may not use this file except in compliance
+  ~ with the License.  You may obtain a copy of the License at
+  ~
+  ~   http://www.apache.org/licenses/LICENSE-2.0
+  ~
+  ~ Unless required by applicable law or agreed to in writing,
+  ~ software distributed under the License is distributed on an
+  ~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+  ~ KIND, either express or implied.  See the License for the
+  ~ specific language governing permissions and limitations
+  ~ under the License.
+  -->
+
+# Revised Integration Tests
+
+This directory builds a Docker image for Druid,
+then usees that image, along with test configuration to run tests.

Review Comment:
   Typo:
   ```suggestion
   then uses that image, along with test configuration to run tests.
   ```



##########
server/src/main/java/org/apache/druid/curator/CuratorModule.java:
##########
@@ -87,14 +74,26 @@ public CuratorFramework makeCurator(ZkEnablementConfig zkEnablementConfig, Curat
 
     RetryPolicy retryPolicy = new BoundedExponentialBackoffRetry(BASE_SLEEP_TIME_MS, MAX_SLEEP_TIME_MS, MAX_RETRIES);
 
-    final CuratorFramework framework = builder
+    return builder
         .ensembleProvider(ensembleProvider)
         .sessionTimeoutMs(config.getZkSessionTimeoutMs())
         .connectionTimeoutMs(config.getZkConnectionTimeoutMs())
         .retryPolicy(retryPolicy)
         .compressionProvider(new PotentiallyGzippedCompressionProvider(config.getEnableCompression()))
         .aclProvider(config.getEnableAcl() ? new SecuredACLProvider() : new DefaultACLProvider())
         .build();
+  }
+
+  @Provides
+  @LazySingleton
+  @SuppressForbidden(reason = "System#err")
+  public CuratorFramework makeCurator(ZkEnablementConfig zkEnablementConfig, CuratorConfig config, EnsembleProvider ensembleProvider, Lifecycle lifecycle)
+  {
+    if (!zkEnablementConfig.isEnabled()) {
+      throw new RuntimeException("Zookeeper is disabled, Can't create CuratorFramework.");

Review Comment:
   Super super nit: 😛
   As we are cleaning up some of the messages,
   ```suggestion
         throw new RuntimeException("Zookeeper is disabled, cannot create CuratorFramework.");
   ```



##########
integration-tests/README.md:
##########
@@ -54,47 +54,85 @@ export APACHE_ARCHIVE_MIRROR_HOST=https://example.com/remote-generic-repo
 
 ## Running tests againt auto brought up Docker containers
 
-> NOTE: This section describes how to start integration tests against docker containers which will be brought up automatically by following commands.
-If you want to buid docker images and run tests separately, see the next section.
+This section describes how to start integration tests against Docker containers which will be brought up automatically by following commands.
+If you want to build Docker images and run tests separately, see the next section.
 
-To run all tests from a test group using docker and mvn run the following command: 
+To run all tests from a test group using Docker and Maven run the following command:
+
+To run all tests from a test group using docker and mvn run the following command:
 (list of test groups can be found at `integration-tests/src/test/java/org/apache/druid/tests/TestNGGroup.java`)
 ```bash
 mvn verify -P integration-tests -Dgroups=<test_group>
 ```
 
-To run only a single test using mvn run the following command:
+### Run a Single Test
+
+To run only a single test using Maven:
+
 ```bash
 mvn verify -P integration-tests -Dgroups=<test_group> -Dit.test=<test_name>
 ```
-The test group should always be set, as certain test setup and cleanup tasks are based on the test group. You can find
-the test group for a given test as an annotation in the respective test class.
 
-Add `-rf :druid-integration-tests` when running integration tests for the second time or later without changing
+Parameters:
+
+* Test Group: Required, as certain test setup and cleanup tasks are based on the test group. You can find
+the test group for a given test as an annotation in the respective test class. A list of test groups can be found at
+`integration-tests/src/test/java/org/apache/druid/tests/TestNGGroup.java`. The annotation uses a string
+constant defined in `TsetNGGroup.java`, be sure to use the constant value, not name. For example, if your test has the the annotation: `@Test(groups = TestNGGroup.BATCH_INDEX)` then use the argument `-Dgroups=batch-index`.

Review Comment:
   Typo:
   ```suggestion
   constant defined in `TestNGGroup.java`, be sure to use the constant value, not name. For example, if your test has the the annotation: `@Test(groups = TestNGGroup.BATCH_INDEX)` then use the argument `-Dgroups=batch-index`.
   ```



##########
integration-tests/README.md:
##########
@@ -54,47 +54,85 @@ export APACHE_ARCHIVE_MIRROR_HOST=https://example.com/remote-generic-repo
 
 ## Running tests againt auto brought up Docker containers
 
-> NOTE: This section describes how to start integration tests against docker containers which will be brought up automatically by following commands.
-If you want to buid docker images and run tests separately, see the next section.
+This section describes how to start integration tests against Docker containers which will be brought up automatically by following commands.
+If you want to build Docker images and run tests separately, see the next section.
 
-To run all tests from a test group using docker and mvn run the following command: 
+To run all tests from a test group using Docker and Maven run the following command:
+
+To run all tests from a test group using docker and mvn run the following command:
 (list of test groups can be found at `integration-tests/src/test/java/org/apache/druid/tests/TestNGGroup.java`)
 ```bash
 mvn verify -P integration-tests -Dgroups=<test_group>
 ```
 
-To run only a single test using mvn run the following command:
+### Run a Single Test
+
+To run only a single test using Maven:
+
 ```bash
 mvn verify -P integration-tests -Dgroups=<test_group> -Dit.test=<test_name>
 ```
-The test group should always be set, as certain test setup and cleanup tasks are based on the test group. You can find
-the test group for a given test as an annotation in the respective test class.
 
-Add `-rf :druid-integration-tests` when running integration tests for the second time or later without changing
+Parameters:
+
+* Test Group: Required, as certain test setup and cleanup tasks are based on the test group. You can find
+the test group for a given test as an annotation in the respective test class. A list of test groups can be found at
+`integration-tests/src/test/java/org/apache/druid/tests/TestNGGroup.java`. The annotation uses a string
+constant defined in `TsetNGGroup.java`, be sure to use the constant value, not name. For example, if your test has the the annotation: `@Test(groups = TestNGGroup.BATCH_INDEX)` then use the argument `-Dgroups=batch-index`.
+
+* Test Name: Use the fully-qualified class name.
+
+* Add `-pl :druid-integration-tests` when running integration tests for the second time or later without changing
 the code of core modules in between to skip up-to-date checks for the whole module dependency tree.
 
-Integration tests can also be run with either Java 8 or Java 11 by adding `-Djvm.runtime=#` to mvn command, where `#`
+* Integration tests can also be run with either Java 8 or Java 11 by adding `-Djvm.runtime=#` to the Maven command, where `#`
 can either be 8 or 11.
 
-Druid's configuration (using Docker) can be overrided by providing `-Doverride.config.path=<PATH_TO_FILE>`. 
+* Druid's configuration (using Docker) can be overridden by providing `-Doverride.config.path=<PATH_TO_FILE>`.
 The file must contain one property per line, the key must start with `druid_` and the format should be snake case.
-Note that when bringing up docker containers through mvn and -Doverride.config.path is provided, additional
-Druid routers for security group integration test (permissive tls, no client auth tls, custom check tls) will not be started.   
+Note that when bringing up Docker containers through Maven and `-Doverride.config.path` is provided, additional
+Druid routers for security group integration test (permissive tls, no client auth tls, custom check tls) will not be started.
+
+### Debugging Test Runs
+
+The integration test process is fragile and can fail for many reasons when run on your machine.
+Here are some suggestions.
+
+#### Workround for Failed Builds
+
+Sometimes the command above may fail for reasons unrelated to the changes you wish to test.
+In such cases, a workaround is to build the code first, then use the next section to run
+individual tests. To build:
+
+```bash
+mvn clean package  -P integration-tests -Pskip-static-checks -Pskip-tests -Dmaven.javadoc.skip=true -T1.0C -nsu
+```
+
+#### Keep the Local Maven Cache Fresh
+
+As you work with issues, you may be tempted to reuse already-built jars. That only works for about 24 hours,
+after which Maven will helpfully start downloading snapshot jars from an upstream repository.
+This is, unfortunately, a feature of the build scripts. The `-nsu` option above tries to force
+Maven to only look locally for snapshot jars.
+
+#### Faster Builds
+
+While the `-pl` switch above (should* )

Review Comment:
   Changes pending?



##########
docker-tests/it-tools/README.md:
##########
@@ -0,0 +1,48 @@
+<!--
+  ~ Licensed to the Apache Software Foundation (ASF) under one
+  ~ or more contributor license agreements.  See the NOTICE file
+  ~ distributed with this work for additional information
+  ~ regarding copyright ownership.  The ASF licenses this file
+  ~ to you under the Apache License, Version 2.0 (the
+  ~ "License"); you may not use this file except in compliance
+  ~ with the License.  You may obtain a copy of the License at
+  ~
+  ~   http://www.apache.org/licenses/LICENSE-2.0
+  ~
+  ~ Unless required by applicable law or agreed to in writing,
+  ~ software distributed under the License is distributed on an
+  ~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+  ~ KIND, either express or implied.  See the License for the
+  ~ specific language governing permissions and limitations
+  ~ under the License.
+  -->
+
+# Testing Tools Ex
+
+This module is a copy of `extensions-core/testing-tools` (module
+name `druid-testing-tools`.)
+
+The testing tools are added to the Druid test Docker image. The
+`druid-testing-tools` module defines most such additions. However,
+`integration-tests` defines a custom mode role which also must be

Review Comment:
   Typo:
   ```suggestion
   `integration-tests` defines a custom node role which also must be
   ```



##########
pom.xml:
##########
@@ -1505,6 +1525,7 @@
                             <!--@TODO After fixing https://github.com/apache/druid/issues/4964 remove this parameter-->
                             -Ddruid.indexing.doubleStorage=double
                         </argLine>
+                        <skipTests>${skipUTs}</skipTests>

Review Comment:
   Is this okay? Wouldn't this end up skipping all tests and not just UTs?



##########
server/src/main/java/org/apache/druid/curator/CuratorModule.java:
##########
@@ -48,35 +48,22 @@
 
 public class CuratorModule implements Module
 {
-  static final String CURATOR_CONFIG_PREFIX = "druid.zk.service";
-
-  static final String EXHIBITOR_CONFIG_PREFIX = "druid.exhibitor.service";
+  private static final Logger log = new Logger(CuratorModule.class);
 
   private static final int BASE_SLEEP_TIME_MS = 1000;
-
   private static final int MAX_SLEEP_TIME_MS = 45000;
-
   private static final int MAX_RETRIES = 29;
 
-  private static final Logger log = new Logger(CuratorModule.class);
-
   @Override
   public void configure(Binder binder)
   {
-    JsonConfigProvider.bind(binder, CURATOR_CONFIG_PREFIX, ZkEnablementConfig.class);
-    JsonConfigProvider.bind(binder, CURATOR_CONFIG_PREFIX, CuratorConfig.class);
-    JsonConfigProvider.bind(binder, EXHIBITOR_CONFIG_PREFIX, ExhibitorConfig.class);
+    JsonConfigProvider.bind(binder, CuratorConfig.CONFIG_PREFIX, ZkEnablementConfig.class);
+    JsonConfigProvider.bind(binder, CuratorConfig.CONFIG_PREFIX, CuratorConfig.class);
+    JsonConfigProvider.bind(binder, ExhibitorConfig.CONFIG_PREFIX, ExhibitorConfig.class);
   }
 
-  @Provides
-  @LazySingleton
-  @SuppressForbidden(reason = "System#err")
-  public CuratorFramework makeCurator(ZkEnablementConfig zkEnablementConfig, CuratorConfig config, EnsembleProvider ensembleProvider, Lifecycle lifecycle)
+  public static CuratorFramework createCurator(CuratorConfig config, EnsembleProvider ensembleProvider)

Review Comment:
   Nit: I guess this method can be private now. Also, does it need to be static?



##########
integration-tests/src/main/java/org/apache/druid/testing/IntegrationTestingConfig.java:
##########
@@ -23,6 +23,19 @@
 import java.util.Map;
 
 /**
+ * Configuration for tests. Opinionated about the shape of the cluster:
+ * there is one or two Coordinators or Overlords, zero or one of
+ * everything else.
+ * <p>
+ * To work in Docker (and K8s) there are to methods per host:

Review Comment:
   Typo
   ```suggestion
    * To work in Docker (and K8s) there are two methods per host:
   ```



##########
server/src/main/java/org/apache/druid/curator/cache/PathChildrenCacheFactory.java:
##########
@@ -65,17 +65,13 @@ public PathChildrenCache make(CuratorFramework curator, String path)
   {
     private static final ThreadFactory DEFAULT_THREAD_FACTORY = ThreadUtils.newThreadFactory("PathChildrenCache");
 
-    private boolean cacheData;
-    private boolean compressed;
+    private boolean cacheData = true;
+    private boolean compressed = false;
     private ExecutorService exec;
-    private boolean shutdownExecutorOnClose;
+    private boolean shutdownExecutorOnClose = true;
 
     public Builder()

Review Comment:
   Nit: Maybe remove the whole constructor?



##########
integration-tests/README.md:
##########
@@ -54,47 +54,85 @@ export APACHE_ARCHIVE_MIRROR_HOST=https://example.com/remote-generic-repo
 
 ## Running tests againt auto brought up Docker containers
 
-> NOTE: This section describes how to start integration tests against docker containers which will be brought up automatically by following commands.
-If you want to buid docker images and run tests separately, see the next section.
+This section describes how to start integration tests against Docker containers which will be brought up automatically by following commands.
+If you want to build Docker images and run tests separately, see the next section.
 
-To run all tests from a test group using docker and mvn run the following command: 
+To run all tests from a test group using Docker and Maven run the following command:
+
+To run all tests from a test group using docker and mvn run the following command:
 (list of test groups can be found at `integration-tests/src/test/java/org/apache/druid/tests/TestNGGroup.java`)
 ```bash
 mvn verify -P integration-tests -Dgroups=<test_group>
 ```
 
-To run only a single test using mvn run the following command:
+### Run a Single Test
+
+To run only a single test using Maven:
+
 ```bash
 mvn verify -P integration-tests -Dgroups=<test_group> -Dit.test=<test_name>
 ```
-The test group should always be set, as certain test setup and cleanup tasks are based on the test group. You can find
-the test group for a given test as an annotation in the respective test class.
 
-Add `-rf :druid-integration-tests` when running integration tests for the second time or later without changing
+Parameters:
+
+* Test Group: Required, as certain test setup and cleanup tasks are based on the test group. You can find
+the test group for a given test as an annotation in the respective test class. A list of test groups can be found at
+`integration-tests/src/test/java/org/apache/druid/tests/TestNGGroup.java`. The annotation uses a string
+constant defined in `TsetNGGroup.java`, be sure to use the constant value, not name. For example, if your test has the the annotation: `@Test(groups = TestNGGroup.BATCH_INDEX)` then use the argument `-Dgroups=batch-index`.
+
+* Test Name: Use the fully-qualified class name.
+
+* Add `-pl :druid-integration-tests` when running integration tests for the second time or later without changing
 the code of core modules in between to skip up-to-date checks for the whole module dependency tree.
 
-Integration tests can also be run with either Java 8 or Java 11 by adding `-Djvm.runtime=#` to mvn command, where `#`
+* Integration tests can also be run with either Java 8 or Java 11 by adding `-Djvm.runtime=#` to the Maven command, where `#`

Review Comment:
   Nit: I guess it was better to call it "the `mvn` command" rather than "the Maven command"



##########
core/src/main/java/org/apache/druid/metadata/MetadataStorageConnectorConfig.java:
##########
@@ -49,6 +51,32 @@
   @JsonProperty("dbcp")
   private Properties dbcpProperties;
 
+  public static MetadataStorageConnectorConfig create(
+      String connectUri,
+      String user,
+      String password,
+      Map<String, Object> properties
+  )
+  {
+    MetadataStorageConnectorConfig config = new MetadataStorageConnectorConfig();
+    if (connectUri != null) {
+      config.connectURI = connectUri;
+    }
+    if (user != null) {
+      config.user = user;
+    }
+    if (password != null) {
+      config.passwordProvider = () -> password;
+    }
+    if (properties != null) {
+      config.dbcpProperties = new Properties();
+      for (Entry<String, Object> entry : properties.entrySet()) {
+        config.dbcpProperties.put(entry.getKey(), entry.getValue());
+      }

Review Comment:
   ```suggestion
         config.dbcpProperties.putAll(properties);
   ```



##########
server/src/main/java/org/apache/druid/curator/CuratorModule.java:
##########
@@ -123,9 +122,7 @@ public void stop()
     return framework;
   }
 
-  @Provides
-  @LazySingleton
-  public EnsembleProvider makeEnsembleProvider(CuratorConfig config, ExhibitorConfig exConfig)
+  public static EnsembleProvider createEnsembleProvider(CuratorConfig config, ExhibitorConfig exConfig)

Review Comment:
   Nit: Same for this method. Does it need to be public static?



##########
integration-tests/README.md:
##########
@@ -54,47 +54,85 @@ export APACHE_ARCHIVE_MIRROR_HOST=https://example.com/remote-generic-repo
 
 ## Running tests againt auto brought up Docker containers
 
-> NOTE: This section describes how to start integration tests against docker containers which will be brought up automatically by following commands.
-If you want to buid docker images and run tests separately, see the next section.
+This section describes how to start integration tests against Docker containers which will be brought up automatically by following commands.
+If you want to build Docker images and run tests separately, see the next section.
 
-To run all tests from a test group using docker and mvn run the following command: 
+To run all tests from a test group using Docker and Maven run the following command:
+
+To run all tests from a test group using docker and mvn run the following command:
 (list of test groups can be found at `integration-tests/src/test/java/org/apache/druid/tests/TestNGGroup.java`)
 ```bash
 mvn verify -P integration-tests -Dgroups=<test_group>
 ```
 
-To run only a single test using mvn run the following command:
+### Run a Single Test
+
+To run only a single test using Maven:
+
 ```bash
 mvn verify -P integration-tests -Dgroups=<test_group> -Dit.test=<test_name>
 ```
-The test group should always be set, as certain test setup and cleanup tasks are based on the test group. You can find
-the test group for a given test as an annotation in the respective test class.
 
-Add `-rf :druid-integration-tests` when running integration tests for the second time or later without changing
+Parameters:
+
+* Test Group: Required, as certain test setup and cleanup tasks are based on the test group. You can find
+the test group for a given test as an annotation in the respective test class. A list of test groups can be found at
+`integration-tests/src/test/java/org/apache/druid/tests/TestNGGroup.java`. The annotation uses a string
+constant defined in `TsetNGGroup.java`, be sure to use the constant value, not name. For example, if your test has the the annotation: `@Test(groups = TestNGGroup.BATCH_INDEX)` then use the argument `-Dgroups=batch-index`.
+
+* Test Name: Use the fully-qualified class name.
+
+* Add `-pl :druid-integration-tests` when running integration tests for the second time or later without changing
 the code of core modules in between to skip up-to-date checks for the whole module dependency tree.
 
-Integration tests can also be run with either Java 8 or Java 11 by adding `-Djvm.runtime=#` to mvn command, where `#`
+* Integration tests can also be run with either Java 8 or Java 11 by adding `-Djvm.runtime=#` to the Maven command, where `#`
 can either be 8 or 11.
 
-Druid's configuration (using Docker) can be overrided by providing `-Doverride.config.path=<PATH_TO_FILE>`. 
+* Druid's configuration (using Docker) can be overridden by providing `-Doverride.config.path=<PATH_TO_FILE>`.
 The file must contain one property per line, the key must start with `druid_` and the format should be snake case.
-Note that when bringing up docker containers through mvn and -Doverride.config.path is provided, additional
-Druid routers for security group integration test (permissive tls, no client auth tls, custom check tls) will not be started.   
+Note that when bringing up Docker containers through Maven and `-Doverride.config.path` is provided, additional
+Druid routers for security group integration test (permissive tls, no client auth tls, custom check tls) will not be started.
+
+### Debugging Test Runs

Review Comment:
   Thanks a lot for including this!



##########
server/src/main/java/org/apache/druid/metadata/SegmentsMetadataManagerConfig.java:
##########
@@ -26,6 +26,8 @@
  */
 public class SegmentsMetadataManagerConfig
 {
+  public static final String PROPERTY_PREFIX = "druid.manager.segments";

Review Comment:
   Nit: The name of a similar field you have used elsewhere is `CONFIG_PREFIX`. Should we stick to the same?



##########
integration-tests/src/main/java/org/apache/druid/testing/IntegrationTestingConfig.java:
##########
@@ -23,6 +23,19 @@
 import java.util.Map;
 
 /**
+ * Configuration for tests. Opinionated about the shape of the cluster:

Review Comment:
   Thanks for adding these!



##########
server/src/main/java/org/apache/druid/discovery/BaseNodeRoleWatcher.java:
##########
@@ -219,12 +219,15 @@ public void cacheInitialized()
         return;
       }
 
-      LOGGER.info("Node watcher of role[%s] is now initialized.", nodeRole.getJsonName());
+      // It is important to take a snapshot here as list of nodes might change by the time listeners process
+      // the changes.
+      List<DiscoveryDruidNode> currNodes = Lists.newArrayList(nodes.values());

Review Comment:
   Nice catch!
   



##########
pom.xml:
##########
@@ -201,6 +207,12 @@
         <module>extensions-contrib/opentelemetry-emitter</module>
         <!-- distribution packaging -->
         <module>distribution</module>
+        <!-- Revised integration tests -->
+        <module>docker-tests/it-tools</module>
+        <module>docker-tests/it-image</module>
+		<module>docker-tests/it-base</module>

Review Comment:
   Super Nit: extra space.



##########
docker-tests/README.md:
##########
@@ -0,0 +1,103 @@
+<!--
+  ~ Licensed to the Apache Software Foundation (ASF) under one
+  ~ or more contributor license agreements.  See the NOTICE file
+  ~ distributed with this work for additional information
+  ~ regarding copyright ownership.  The ASF licenses this file
+  ~ to you under the Apache License, Version 2.0 (the
+  ~ "License"); you may not use this file except in compliance
+  ~ with the License.  You may obtain a copy of the License at
+  ~
+  ~   http://www.apache.org/licenses/LICENSE-2.0
+  ~
+  ~ Unless required by applicable law or agreed to in writing,
+  ~ software distributed under the License is distributed on an
+  ~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+  ~ KIND, either express or implied.  See the License for the
+  ~ specific language governing permissions and limitations
+  ~ under the License.
+  -->
+
+# Revised Integration Tests
+
+This directory builds a Docker image for Druid,
+then usees that image, along with test configuration to run tests.
+This version greatly evolves the integration tests from the earlier
+form. See the last section for details.
+
+## Run the Tests
+
+At present, two of the test groups are converted to the new format as
+a proof-of-concept. Run the new ITs with:
+
+```bash
+mvn clean install -P dist,test-image,docker-tests \

Review Comment:
   Is there a way to run a single test from the console or can we only use the IDE for that?



##########
pom.xml:
##########
@@ -1177,6 +1190,13 @@
                         <exclude>org/apache/druid/benchmark/**/*</exclude>  <!-- benchmarks -->
                         <exclude>org/apache/druid/**/*Benchmark*</exclude>  <!-- benchmarks -->
                         <exclude>org/apache/druid/testing/**/*</exclude>  <!-- integration-tests -->
+                        <!-- Would be better to put these IT items in a distinct package rather than
+                             reusing generic packages. Else exclusion is tedious. -->
+                        <exclude>org/apache/druid/server/coordination/ServerManagerForQueryErrorTest.class</exclude>
+                        <exclude>org/apache/druid/guice/SleepModule.class</exclude>
+                        <exclude>org/apache/druid/guice/CustomNodeRoleClientModule.class</exclude>
+                        <exclude>org/apache/druid/cli/CustomNodeRoleCommandCreator.class</exclude>
+                        <exclude>org/apache/druid/cli/QueryRetryTestCommandCreator.class</exclude>

Review Comment:
   Now that you have put them in a separate package, I guess these exclusions are not needed anymore?



##########
docker-tests/it-tools/README.md:
##########
@@ -0,0 +1,48 @@
+<!--
+  ~ Licensed to the Apache Software Foundation (ASF) under one
+  ~ or more contributor license agreements.  See the NOTICE file
+  ~ distributed with this work for additional information
+  ~ regarding copyright ownership.  The ASF licenses this file
+  ~ to you under the Apache License, Version 2.0 (the
+  ~ "License"); you may not use this file except in compliance
+  ~ with the License.  You may obtain a copy of the License at
+  ~
+  ~   http://www.apache.org/licenses/LICENSE-2.0
+  ~
+  ~ Unless required by applicable law or agreed to in writing,
+  ~ software distributed under the License is distributed on an
+  ~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+  ~ KIND, either express or implied.  See the License for the
+  ~ specific language governing permissions and limitations
+  ~ under the License.
+  -->
+
+# Testing Tools Ex

Review Comment:
   Isn't this renamed to `it-tools` now?



##########
docker-tests/it-base/src/test/java/org/apache/druid/testing2/cluster/ClusterClient.java:
##########
@@ -0,0 +1,333 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.testing2.cluster;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.java.util.common.RE;
+import org.apache.druid.java.util.common.StringUtils;
+import org.apache.druid.java.util.common.jackson.JacksonUtils;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.java.util.http.client.HttpClient;
+import org.apache.druid.java.util.http.client.Request;
+import org.apache.druid.java.util.http.client.response.StatusResponseHandler;
+import org.apache.druid.java.util.http.client.response.StatusResponseHolder;
+import org.apache.druid.server.DruidNode;
+import org.apache.druid.testing.guice.TestClient;
+import org.apache.druid.testing2.config.ResolvedConfig;
+import org.apache.druid.testing2.config.ResolvedDruidService;
+import org.apache.druid.testing2.config.ResolvedService.ResolvedInstance;
+import org.jboss.netty.handler.codec.http.HttpMethod;
+import org.jboss.netty.handler.codec.http.HttpResponseStatus;
+
+import javax.inject.Inject;
+
+import java.io.IOException;
+import java.net.URL;
+import java.util.Map;
+
+/**
+ * Client to the Druid cluster described by the test cluster
+ * configuration.
+ */
+public class ClusterClient

Review Comment:
   Suggestion: Rename to `DruidClusterClient`



##########
docker-tests/it-base/src/test/java/org/apache/druid/test/TestClusterConfig.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.test;
+
+import org.apache.druid.testing2.config.ClusterConfig;
+import org.apache.druid.testing2.config.ClusterConfig.ClusterType;
+import org.apache.druid.testing2.config.ResolvedConfig;
+import org.apache.druid.testing2.config.ResolvedDruidService;
+import org.apache.druid.testing2.config.ResolvedMetastore;
+import org.apache.druid.testing2.config.ResolvedService.ResolvedZk;
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+
+/**
+ * Sanity check of an example YAML config file using the Java
+ * deserialization classes.
+ */
+public class TestClusterConfig

Review Comment:
   Suggestion: Rename to `DruidClusterConfigTest` or `DruidClusterConfigSanityTest`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org