Migrate query JDBC, cancel and system tables tests, cleanup and refactor helper classes #18805

Fly-Style · 2025-12-03T15:06:02Z

Description

This patch continues the effort to move query group tests to the embedded test suite, currently, we have migrated ITSqlCancelTest, ITSystemTableQueryTest, ITTwitterQueryTest.

Note: TLS-related tests require more effort to support, so we decided to implement them in subsequent patches.

This PR has:

been self-reviewed.
added integration tests.

…er test methods

kfaraz

Thanks for the changes, @Fly-Style !
The tests look much better now.

I have left some minor suggestions. Please let me know what you think.

kfaraz · 2025-12-03T16:33:58Z

embedded-tests/src/test/java/org/apache/druid/testing/embedded/query/JdbcQueryTest.java

+        // Add in the consecutive patch
+        // StringUtils.format(
+        //    TLS_CONNECTION_TEMPLATE,
+        //    config.getRouterTLSUrl(),
+        //    sslConfig.getTrustStorePath(),
+        //    sslConfig.getTrustStorePasswordProvider().getPassword(),
+        //    sslConfig.getKeyStorePath(),
+        //    sslConfig.getKeyStorePasswordProvider().getPassword(),
+        //    sslConfig.getKeyManagerPasswordProvider().getPassword()
+        //),
+        // StringUtils.format(
+        //    TLS_CONNECTION_TEMPLATE,
+        //    config.getBrokerTLSUrl(),
+        //    sslConfig.getTrustStorePath(),
+        //    sslConfig.getTrustStorePasswordProvider().getPassword(),
+        //    sslConfig.getKeyStorePath(),
+        //    sslConfig.getKeyStorePasswordProvider().getPassword(),
+        //    sslConfig.getKeyManagerPasswordProvider().getPassword()
+        // )


Please remove the commented out code. We can just add one line to the javadoc of this class that mentions that the TLS flavor for this test will be added later.

kfaraz · 2025-12-03T16:34:41Z

embedded-tests/src/test/java/org/apache/druid/testing/embedded/query/JdbcQueryTest.java

-    dataLoaderHelper.waitUntilDatasourceIsReady("wikipedia");
-    dataLoaderHelper.waitUntilDatasourceIsReady("twitterstream");
+
+    tableName = ingestBasicData();


Nit: In Druid, we generally use the term datasource. Maybe rename this field to testDataSource or similar?

Rename pending?

kfaraz · 2025-12-03T16:53:59Z

embedded-tests/src/test/java/org/apache/druid/testing/embedded/query/SqlQueryCancelTest.java

+    // Wait until the sqlLifecycle is authorized and registered
+    Thread.sleep(500L);


Is this needed in the embedded test framework too?

In that case, unfortunately, yes. We need to give query some time to spin up :)

oh, I see. Could you we wait on a metric instead, say sqlQuery/planningTimeMs?
There could be other metrics too which mark the execution start of the query.

embedded-tests/src/test/java/org/apache/druid/testing/embedded/query/SqlQueryCancelTest.java

embedded-tests/src/test/java/org/apache/druid/testing/embedded/query/JdbcQueryTest.java

embedded-tests/src/test/java/org/apache/druid/testing/embedded/query/SystemTableQueryTest.java

kfaraz

Looks good! Only blocking comments are these:

Use ingestBasicData() in SystemTableQueryTest
Remove sleep() from SqlQueryCancelTest by using some appropriate metric

kfaraz · 2025-12-04T08:35:42Z

embedded-tests/src/test/java/org/apache/druid/testing/embedded/query/JdbcQueryTest.java

-    dataLoaderHelper.waitUntilDatasourceIsReady("wikipedia");
-    dataLoaderHelper.waitUntilDatasourceIsReady("twitterstream");
+
+    tableName = ingestBasicData();


Rename pending?

kfaraz · 2025-12-04T08:38:07Z

embedded-tests/src/test/java/org/apache/druid/testing/embedded/query/QueryTestBase.java

+    URL url;
+    try {
+      url = new URL(endpoint);
+    }
+    catch (MalformedURLException e) {
+      throw new AssertionError("Malformed URL");
+    }
+
+    Request request = new Request(HttpMethod.POST, url);
+    request.addHeader("Content-Type", MediaType.APPLICATION_JSON);
+    request.setContent(query.getBytes(StandardCharsets.UTF_8));
+    return httpClientRef.go(request, StatusResponseHandler.getInstance());


We can try to augment the EmbeddedClusterApis to provide this functionality and also some API to cancel queries in a follow up PR. Not needed right now.

integration-tests/docker/environment-configs/historical-for-query-error-test

embedded-tests/src/test/java/org/apache/druid/testing/embedded/query/SystemTableQueryTest.java

kfaraz

Thanks for fixing these up, @Fly-Style !
Happy to get rid of these ITs! 😛

kfaraz · 2025-12-05T03:04:40Z

HashPartitionMultiPhaseParallelIndexingTest is failing consistently on this PR.
It is definitely not related to this PR, but we need it to pass so that other checks can be performed.
Need to check if this test is failing on master too.

kfaraz · 2025-12-05T07:00:20Z

Merging this off as failures are unrelated.

…dded tests (#18867) Changes: - Replace `ITAppendBatchIndexTest` with a new method in `IndexParallelTaskTest` - Migrate `ITCustomCoordinatorDutiesTest` to embedded tests as `KillSupervisorsCustomDutyTest` - Remove the following test groups since all the tests for that group have already been migrated - `input-source` (#18805, #18752) - `cds-task-schema-publish-disabled` - `cds-coordinator-metadata-query-disabled`

Fly-Style added 16 commits November 28, 2025 14:35

Stage 1: move QueryErrorTest, create a base class for the convenience

1ffeec3

Move sleep to 'fake' happens-before relationship of beforeAll and oth…

36e1d90

…er test methods

Return back broadcast test

2e4bec3

Migrate query retry on missing segments test

0d5cde4

Don't use parametrize test, duplicate usage instead

c6612ce

Small cleanup

ed2c0a3

Merge branch 'master' into it-to-embed-query

52bcbcd

WIP

2db93a8

Fix checkstyle issue

9b43e1c

Merge branch 'it-to-embed-query' into it-to-embed-query-full

9769ebc

WIP-2

d2a26a0

Successfully migrated query cancellation test to embedded suite

8e09e26

Migreate system table query test

bf91e0f

Migrate JDBC test too

144cdb6

Merge branch 'master' into it-to-embed-query-full

c6be7ac

Post-merge merge

d514715

github-actions bot added the GHA label Dec 3, 2025

kfaraz reviewed Dec 3, 2025

View reviewed changes

Fly-Style added 2 commits December 3, 2025 19:47

Eliminate test flakiness

072b7a2

Address review comments

0a64c9b

kfaraz reviewed Dec 4, 2025

View reviewed changes

Fly-Style added 2 commits December 4, 2025 10:52

Enhance the test

9c8baa5

Address review comments - 2

03e22ae

kfaraz reviewed Dec 4, 2025

View reviewed changes

embedded-tests/src/test/java/org/apache/druid/testing/embedded/query/SystemTableQueryTest.java Outdated Show resolved Hide resolved

Last comment addressed

df9672b

kfaraz approved these changes Dec 4, 2025

View reviewed changes

Merge branch 'master' into it-to-embed-query-full

381f9f6

kfaraz merged commit af4e63d into apache:master Dec 5, 2025
94 of 97 checks passed

kfaraz mentioned this pull request Dec 27, 2025

Migrate ITAppendBatchIndexTest, ITCustomCoordinatorDutiesTest to embedded tests #18867

Merged

10 tasks

		// Wait until the sqlLifecycle is authorized and registered
		Thread.sleep(500L);

Migrate query JDBC, cancel and system tables tests, cleanup and refactor helper classes #18805

Migrate query JDBC, cancel and system tables tests, cleanup and refactor helper classes #18805

Uh oh!

Conversation

Fly-Style commented Dec 3, 2025

Description

Uh oh!

kfaraz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfaraz Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kfaraz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kfaraz left a comment

Choose a reason for hiding this comment

Uh oh!

kfaraz commented Dec 5, 2025

Uh oh!

kfaraz commented Dec 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kfaraz Dec 4, 2025 •

edited

Loading