[GitHub] [druid] jon-wei commented on a change in pull request #9724: Add integration tests for kafka ingestion

[GitBox <gi...@apache.org>]

jon-wei commented on a change in pull request #9724:
URL: https://github.com/apache/druid/pull/9724#discussion_r411853712



##########
File path: integration-tests/README.md
##########
@@ -303,3 +314,13 @@ This will tell the test framework that the test class needs to be constructed us
 2) FromFileTestQueryHelper - reads queries with expected results from file and executes them and verifies the results using ResultVerifier
 
 Refer ITIndexerTest as an example on how to use dependency Injection
+
+### Running test methods in parallel
+By default, test methods in a test class will be run in sequential order one at a time. Test methods for a given test 
+class can be set to run in parallel (multiple test methods of the given class running at the same time) by excluding
+the given class/package from the "AllSerializedTests" test tag section and including it in the "AllParallelizedTests" 
+test tag section in integration-tests/src/test/resources/testng.xml  
+Please be mindful when adding tests to the "AllParallelizedTests" test tag that the tests can run in parallel with
+other tests at the same time. i.e. test does not modify/restart/stop the druid cluster or other dependecy containers,

Review comment:
       dependecy -> dependency

##########
File path: integration-tests/README.md
##########
@@ -303,3 +314,13 @@ This will tell the test framework that the test class needs to be constructed us
 2) FromFileTestQueryHelper - reads queries with expected results from file and executes them and verifies the results using ResultVerifier
 
 Refer ITIndexerTest as an example on how to use dependency Injection
+
+### Running test methods in parallel
+By default, test methods in a test class will be run in sequential order one at a time. Test methods for a given test 
+class can be set to run in parallel (multiple test methods of the given class running at the same time) by excluding
+the given class/package from the "AllSerializedTests" test tag section and including it in the "AllParallelizedTests" 
+test tag section in integration-tests/src/test/resources/testng.xml  
+Please be mindful when adding tests to the "AllParallelizedTests" test tag that the tests can run in parallel with
+other tests at the same time. i.e. test does not modify/restart/stop the druid cluster or other dependecy containers,
+test does not use excessive memory straving other concurent task, test does not modify and/or use other task, 

Review comment:
       straving -> starving

##########
File path: integration-tests/pom.xml
##########
@@ -385,6 +367,7 @@
                                 <configuration>
                                     <environmentVariables>
                                     <DRUID_INTEGRATION_TEST_START_HADOOP_DOCKER>${start.hadoop.docker}</DRUID_INTEGRATION_TEST_START_HADOOP_DOCKER>
+                                    <DRUID_INTEGRATION_TEST_SKIP_START_DOCKER>${skip.start.docker}</DRUID_INTEGRATION_TEST_SKIP_START_DOCKER>

Review comment:
       👍 

##########
File path: integration-tests/src/main/java/org/apache/druid/testing/utils/KinesisAdminClient.java
##########
@@ -129,18 +132,28 @@ public void updateShardCount(String streamName, int newShardCount, boolean block
     }
   }
 
+  @Override
   public boolean isStreamActive(String streamName)
   {
     StreamDescription streamDescription = getStreamDescription(streamName);
     return verifyStreamStatus(streamDescription, StreamStatus.ACTIVE);
   }
 
+  @Override
   public int getStreamShardCount(String streamName)
   {
     StreamDescription streamDescription = getStreamDescription(streamName);
     return getStreamShardCount(streamDescription);
   }
 
+  @Override
+  public boolean verfiyShardCountUpdated(String streamName, int oldShardCount, int newShardCount)
+  {
+    int actualShardCount = getStreamShardCount(streamName);
+    return actualShardCount == oldShardCount + newShardCount;

Review comment:
       Suggest adding a brief comment here noting that Kinesis doesn't immediately drop the old shards after the resharding which is why the counts are summed

##########
File path: integration-tests/src/test/java/org/apache/druid/tests/indexer/AbstractStreamIndexingTest.java
##########
@@ -0,0 +1,439 @@
+/*
+ * 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.tests.indexer;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.inject.Inject;
+import org.apache.druid.indexing.overlord.supervisor.SupervisorStateManager;
+import org.apache.druid.java.util.common.DateTimes;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.testing.utils.DruidClusterAdminClient;
+import org.apache.druid.testing.utils.ITRetryUtil;
+import org.apache.druid.testing.utils.StreamAdminClient;
+import org.apache.druid.testing.utils.StreamEventWriter;
+import org.apache.druid.testing.utils.WikipediaStreamEventStreamGenerator;
+import org.joda.time.DateTime;
+import org.joda.time.format.DateTimeFormat;
+import org.joda.time.format.DateTimeFormatter;
+
+import java.io.Closeable;
+import java.util.Map;
+import java.util.UUID;
+import java.util.function.Function;
+
+public abstract class AbstractStreamIndexingTest extends AbstractITBatchIndexTest

Review comment:
       Hmm, it's a bit strange that the stream indexing test extends something called "batch index test", maybe the base "batch index" class is generic enough that it should be renamed to something else, or some other refactoring




----------------------------------------------------------------
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.

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