Patch for the ITs for MSQ #1

LakshSingla · 2022-08-23T06:32:50Z

Description

Fixed the bug ...

Renamed the class ...

Added a forbidden-apis entry ...

This PR has:

been self-reviewed.
- using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
added documentation for new or modified features or behaviors.
added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
added or updated version, license, or notice information in licenses.yaml
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
added integration tests.
been tested in a test Druid cluster.

Support for all of the IntegrationTestingConfig properties Wrapper script for IT operations Additional documentation Build fix

Improved env var-based configuration Test createion guide Enable new ITs in travis Parameterized tests

cryptoe · 2022-08-24T06:30:29Z

integration-tests-ex/cases/src/test/java/org/apache/druid/testsEx/msq/ITMultiStageQuery.java

+  public void test() throws Exception
+  {
+    String query =
+        "INSERT INTO dst SELECT *\n"


This should be picked up from a file using a http data source

It is not necessary to put queries in a file. Existing tests use files, but only because they have many queries. However, since this query is rather complex, it might be easier to maintain if it is in a file.

The existing format may not be a good fit for MSQ tests, so we might want to create a simplified form for use here.

cryptoe · 2022-08-24T06:30:49Z

integration-tests-ex/cases/src/test/java/org/apache/druid/testsEx/msq/ITMultiStageQuery.java

+    String resultsQuery = "SELECT * FROM dst";
+    String resultsTaskId = msqHelper.submitMsqTask(resultsQuery);
+    msqHelper.pollTaskIdForCompletion(resultsTaskId, 0);
+    msqHelper.compareResults(resultsTaskId, new MsqQueryWithResults(


Results should also be served from a file.

This is where we can use the existing format which holds both the query and expected results.

…into msq-first-it

paul-rogers

Thanks for this starter set of MSQ tests. Sorry you had to wrestle with the in-flight IT code.

Suggestion: shift your work to use the Apache Druid code. The IT stuff there had many last-minute tweaks and should now be a bit easier to use.

paul-rogers · 2022-08-29T07:19:05Z

.travis.yml

@@ -46,7 +46,7 @@ addons:
 # Add various options to make 'mvn install' fast and skip javascript compile (-Ddruid.console.skip=true) since it is not
 # needed. Depending on network speeds, "mvn -q install" may take longer than the default 10 minute timeout to print any
 # output.  To compensate, use travis_wait to extend the timeout.
-install: ./check_test_suite.py && travis_terminate 0  || echo 'Running Maven install...' && MAVEN_OPTS='-Xmx3000m' travis_wait 15 ${MVN} clean install -q -ff -pl '!distribution,!:it-tools,!:it-image' ${MAVEN_SKIP} ${MAVEN_SKIP_TESTS} -T1C && ${MVN} install -q -ff -pl 'distribution' ${MAVEN_SKIP} ${MAVEN_SKIP_TESTS}
+install: ./check_test_suite.py && travis_terminate 0  || echo 'Running Maven install...' && MAVEN_OPTS='-Xmx3000m' travis_wait 15 ${MVN} clean install -q -ff -pl '!distribution,!:druid-it-tools,!:druid-it-image,!:druid-it-cases' ${MAVEN_SKIP} ${MAVEN_SKIP_TESTS} -T1C && ${MVN} install -q -ff -pl 'distribution' ${MAVEN_SKIP} ${MAVEN_SKIP_TESTS}


This should be fixed if you rebase on the latest Imply master.

paul-rogers · 2022-08-29T07:20:49Z

.travis.yml

+      name: "(Compile=openjdk8, Run=openjdk8) batch index integration test with Indexer (new)"
+      env: JVM_RUNTIME='-Djvm.runtime=8' USE_INDEXER='indexer'
+      script: ./it.sh travis BatchIndex
+


Suggestion: now that MSQ and the IT code is in Apache master, change this PR to work off of Apache. That way, you can wait for me to merge the Jenkins changes before attempting to run the tests in Imply. The Jenkins stuff was a real hassle: no reason for you to rediscover that pain.

paul-rogers · 2022-08-29T07:22:19Z

integration-tests-ex/README.md

 ```bash
-mvn clean package -P dist,skip-static-checks,skip-tests -Dmaven.javadoc.skip=true -T1.0C
+mvn clean package -P dist $MAVEN_IGNORE -T1.0C


In the run-up to the merge, this was all replaced with a script, since it became silly to keep copy/pasting the same command lines over & over. You'll get that benefit when you rebase.

paul-rogers · 2022-08-29T07:23:59Z

integration-tests-ex/cases/cluster/MultiStageQuery/docker-compose.yaml

+      file: ../Common/druid.yaml
+      service: broker
+    environment:
+      - DRUID_INTEGRATION_TEST_GROUP=${DRUID_INTEGRATION_TEST_GROUP}


This environment variable is no longer needed. At present, nothing in the container makes use of the test group.

paul-rogers · 2022-08-29T07:27:40Z

integration-tests-ex/cases/cluster/MultiStageQuery/docker-compose.yaml

+      config:
+        - subnet: 172.172.172.0/24
+
+services:


This cluster looks pretty generic. It seems to be basically the same as BatchIndex. Seems we should define a generic cluster setup then point this test (and BatchIndexer and InputFormat) at that definition. I've made a note to clean this up.

paul-rogers · 2022-08-29T07:29:31Z

integration-tests-ex/cases/src/test/java/org/apache/druid/testsEx/msq/ITMultiStageQuery.java

+  public void test() throws Exception
+  {
+    String query =
+        "INSERT INTO dst SELECT *\n"


It is not necessary to put queries in a file. Existing tests use files, but only because they have many queries. However, since this query is rather complex, it might be easier to maintain if it is in a file.

The existing format may not be a good fit for MSQ tests, so we might want to create a simplified form for use here.

paul-rogers · 2022-08-29T07:30:14Z

integration-tests-ex/cases/src/test/java/org/apache/druid/testsEx/msq/ITMultiStageQuery.java

+    String resultsQuery = "SELECT * FROM dst";
+    String resultsTaskId = msqHelper.submitMsqTask(resultsQuery);
+    msqHelper.pollTaskIdForCompletion(resultsTaskId, 0);
+    msqHelper.compareResults(resultsTaskId, new MsqQueryWithResults(


This is where we can use the existing format which holds both the query and expected results.

paul-rogers · 2022-08-29T07:33:36Z

integration-tests-ex/cases/src/test/resources/cluster/AzureDeepStorage/docker.yaml

+# druid.test.config.dockerIp is used by some older test code. Remove
+# it when that code is updated.
+properties:
+  druid.global.http.numMaxThreads: 3


Most of these properties are not needed by this test. And, some of these are a bit of a confusion: these properties are for the test client, not the server. Only the thread settings are needed, and only to keep the client from creating a large number of threads which clutter the IDE experience unnecessarily.

LakshSingla · 2022-08-30T06:08:42Z

@paul-rogers Thanks for the review. I have opened the PR in apache/druid/master apache#12992. Apologies for the confusion and not intimating this earlier. I will try to incorporate the review comments from this PR there.

paul-rogers and others added 8 commits August 16, 2022 23:45

Refine the IT framework

ac14060

Support for all of the IntegrationTestingConfig properties Wrapper script for IT operations Additional documentation Build fix

Final revsions

e034f60

Improved env var-based configuration Test createion guide Enable new ITs in travis Parameterized tests

Disabled flaky tests to get a clean build

7e98bdb

Build fixes

754ba9c

first test, serde causing problems

1a5af17

serde working

c0357d2

insert and select check

51170bc

Merge fix

9ac998f

cryptoe reviewed Aug 24, 2022

View reviewed changes

LakshSingla added 4 commits August 24, 2022 16:10

Add cluster annotations for MSQ test cases

b99fcc2

Merge branch '220325-docker' of https://github.com/paul-rogers/druid …

e2aef43

…into msq-first-it

Add cluster config for MSQ

5a51441

Add MSQ config to the pom.xml

8daef61

paul-rogers reviewed Aug 29, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Patch for the ITs for MSQ #1

Patch for the ITs for MSQ #1

LakshSingla commented Aug 23, 2022

cryptoe Aug 24, 2022

paul-rogers Aug 29, 2022

cryptoe Aug 24, 2022

paul-rogers Aug 29, 2022

paul-rogers left a comment

paul-rogers Aug 29, 2022

paul-rogers Aug 29, 2022

paul-rogers Aug 29, 2022

paul-rogers Aug 29, 2022

paul-rogers Aug 29, 2022

paul-rogers Aug 29, 2022

paul-rogers Aug 29, 2022

paul-rogers Aug 29, 2022

LakshSingla commented Aug 30, 2022

Patch for the ITs for MSQ #1

Are you sure you want to change the base?

Patch for the ITs for MSQ #1

Conversation

LakshSingla commented Aug 23, 2022

Description

Fixed the bug ...

Renamed the class ...

Added a forbidden-apis entry ...

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-rogers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LakshSingla commented Aug 30, 2022