Refactor: Rename DataSourceTaskIdHolder and extract load spec holder to its own class #18732

abhishekrb19 · 2025-11-10T05:04:48Z

Refactor:

Remove named injections of DataSourceTaskIdHolder in favor of:
- TaskHolder interface: implemented by PeonTaskHolder for peon processes and by NoopTaskHolder for all other servers, where the methods return null. The alternative was to throw UnsupportedExceptions in NoopTaskHolder and have callers verify type, but that made the usages a bit difficult and the callers would still have to effectively assign null anyway.
- LoadSpecHolder interface: implemented by PeonLoadSpecHolder for peon processes and DefaultLoadSpecHolder for all other servers.
A module DefaultServerHolderModule that binds the above modules as part of the core injector module for all servers except CliPeon. For CliPeon, the binding to PeonTaskHolder and PeonLoadSpecHolder is explicitly done in CliPeon itself.

Some implementation specific choices have been documented as javadocs in the code. In the future, we can extend TaskHolder to also include groupId, taskType, etc., from the Task and add those dimensions to all the metric modules as applicable.

This PR has:

been self-reviewed.
a release note entry in the PR description.
added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
been tested in a test Druid cluster.

…ss to avoid confusion.

server/src/main/java/org/apache/druid/query/lookup/LookupListeningAnnouncerConfig.java

kfaraz

Thanks for putting this together, @abhishekrb19 !

I had a slightly different approach in mind.

In the original class, DatasourceTaskIdHolder all the fields are derived from Task itself, so we should probably just:

Rename it to PeonTaskHolder to denote that this class is meaningful only for a peon and that it contains the task that the peon is running
Just keep one field inside it Task task and add method getTask()
Retain the original utility methods getDatasource(), getTaskId(), getLookupLoadinSpec() etc.
Create the PeonTaskHolder in a @Provides method in CliPeon.
In non peon servers, bind the PeonTaskHolder to null.

Let me know if this would make sense.

abhishekrb19 · 2025-11-10T07:52:51Z

Thanks for the review, @kfaraz!

I think the approach of keeping one renamed class would work well and makes sense. Just one caveat: the Task class is in the indexing-service module, so we can’t use it directly in the server module. However, we could create a constructor that accepts all the task properties of interest and have it @Provides from CliPeon like you suggested, along with a default constructor for all other servers. This way, we can remove all the individual @Named annotation bindings from CliPeon to DatasourceTaskIdHolder. Does that work?

kfaraz · 2025-11-10T08:07:30Z

Ah, I see, thanks for clarifying that, @abhishekrb19 ! All the current usages of DatasourceTaskIdHolder are indeed in the druid-server module.

In that case, I think we should add an interface TaskHolder to druid-server and use it wherever needed.
Bind the TaskHolder to a PeonTaskHolder in CliPeon and bind it elsewhere to a NoopTaskHolder which throws UnsupportedOperationException upon calling any method. I feel this clarifies the intent better and makes it more extensible for the future.

Sound good?

abhishekrb19 · 2025-11-20T06:29:51Z

@kfaraz sorry I ran into some hiccups with the bindings last week and didn't get a chance to take a closer look, but I'm hoping to revive this change again soon.

Btw I noticed this deprecated comment on EmbeddedMiddleManager, so most (or all?) the tests seem to use the indexer for good reasons. Given that there are some subtle differences between indexers and MMs, do you have any suggestions for embedded tests that use MMs?

At the very least, I was thinking of updating some of the embedded tests to use EmbeddedMiddleManager instead of EmbeddedIndexer locally. I’m also planning to add some unit tests.

kfaraz · 2025-11-20T06:48:08Z

@kfaraz sorry I ran into some hiccups with the bindings last week and didn't get a chance to take a closer look, but I'm hoping to revive this change again soon.

No worries, @abhishekrb19 ! 🙂

Given that there are some subtle differences between indexers and MMs, do you have any suggestions for embedded tests that use MMs?

If the purpose is to test out peons in an embedded test setup, you could do a couple of things:

Use EmbeddedMiddleManager and annotate it @SuppressForbidden(reason = "EmbeddedMiddleManager#init")
Use the k3s environment with the help of K3sClusterResource, e.g. KubernetesClusterDockerTest (These tests would run in the docker-tests GHA workflow. It is also possible to run them locally after setting the druid.testing.docker.image property.)

For local testing, please feel free to update any test to use an MM instead of Indexer. Most of them should work fine for both Indexer and MM.
But while committing, I would advise not to change any of the existing tests to use MMs (since Indexers are faster and easier to debug) and add new MM-flavored variants of any test where necessary.

Please let me know if this meets yours needs.

abhishekrb19 · 2025-11-20T23:44:40Z

Sounds good, thanks for the pointer @kfaraz! I will give that a try

server/src/main/java/org/apache/druid/query/lookup/LookupListeningAnnouncerConfig.java

        "Cannot specify both `lookupTier` and `lookupTierIsDatasource`"
    );
-    final String lookupTier = lookupTierIsDatasource ? dataSourceTaskIdHolder.getDataSource() : this.lookupTier;
+    final String lookupTier = lookupTierIsDatasource ? taskHolder.getDataSource() : this.lookupTier;


The best fix is to rename the local variable lookupTier in the getLookupTier() method to a different name, such as tierToReturn or effectiveLookupTier. This removes any ambiguity between the field and the local variable. This change should only be performed in the method getLookupTier() within LookupListeningAnnouncerConfig.java, specifically where the local variable is declared and used. No imports or additional definitions are necessary, as it's a straightforward renaming.

services/src/test/java/org/apache/druid/cli/CliPeonTest.java

This reverts commit 58185c0.

server/src/test/java/org/apache/druid/query/lookup/LookupListeningAnnouncerConfigTest.java

+    final Injector injector = Initialization.makeInjectorWithModules(
+        GuiceInjectors.makeStartupInjector(),
+        ImmutableList.of(
+            (Module) binder -> JsonConfigProvider.bindInstance(
+                binder,
+                Key.get(DruidNode.class, Self.class),
+                new DruidNode("test-inject", null, false, null, null, true, false)
+            ),
+            new LookupModule()
+        ));


To address the issue, we need to replace the usage of the deprecated Initialization.makeInjectorWithModules method (line 174) with its recommended alternative. According to the Druid source code and upgrade notes, the recommended replacement is generally the static utility method GuiceInjectors.makeInjectorWithModules, which provides the same API for creating an Injector from an existing startup injector and extra modules.

Specifically, you should:

Replace the invocation of Initialization.makeInjectorWithModules(...) with GuiceInjectors.makeInjectorWithModules(...) in the test method testLookupTierDefaultsForNonPeonServers.

No changes to method parameters should be needed, as both methods accept the same arguments.

All necessary imports are already present (GuiceInjectors is imported at line 27).

No further changes are required elsewhere in the provided snippet.

…f checks, etc

kfaraz

Left some final suggestions, otherwise looks good. Thanks for incorporating the changes, @abhishekrb19 !

server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java

kfaraz · 2025-11-28T07:00:21Z

server/src/main/java/org/apache/druid/server/metrics/MetricsModule.java

+    // CliPeon.runTask and TaskHolder.getDataSource()/TaskHolder.getTaskId(). The reason for this is unclear
+    // but by injecting TaskPropertiesHolder early this cycle is avoided.


Is this true even after the changes in this PR?

Ah yes, good point, they're no longer required. I've removed the changes originally included in #16140 and added some tests that would otherwise fail if we remove this "hack".

server/src/main/java/org/apache/druid/server/metrics/MetricsModule.java

kfaraz · 2025-11-28T07:03:11Z

server/src/main/java/org/apache/druid/server/metrics/MonitorsConfig.java

-      @Nullable final String datasource,
-      @Nullable final String taskId
-  )
+  public static Map<String, String[]> mapOfTaskHolderDimensions(final TaskHolder taskHolder)


I think it would now make sense to move this method into TaskHolder itself. The map would be created just once when the TaskHolder instance is created.

Then we can just call taskHolder.getMetricDimensions() and get the same immutable wherever needed.

kfaraz · 2025-11-28T07:09:20Z

services/src/main/java/org/apache/druid/cli/PeonTaskHolder.java

+  private Provider<Task> taskProvider;
+
+  @Inject
+  public PeonTaskHolder(


Suggestion: Do not use @Inject with this constructor and just pass a Task into it instead of a Provider.
Use an @Provides method in CliPeon to bind it correctly.

@Provides public TaskHolder getTaskHolder(Task task) { return new PeonTaskHolder(task); }

There are cyclic dependencies involving Task when it contains a TransformSpec, an AggregatorFactory or a monitor, which will pull in LookupModule, MetricsModule, etc, which depend on these holders. I’ve expanded the Javadoc to explain this in more detail. To break the cycle, this binding needs to use a Provider
instead of direct injection. Switching to @Provides pattern will break UTs and some ITs with a cyclic dependency Guice instantiation error:

End of classname legend: ======================== at org.apache.druid.cli.CliPeon.run(CliPeon.java:413) at org.apache.druid.cli.Main.main(Main.java:111) Caused by: java.lang.RuntimeException: com.google.inject.CreationException: Unable to create injector, see the following errors: 1) [Guice/ErrorInCustomProvider]: RuntimeException: ValueInstantiationException: Cannot construct instance of `ExpressionTransform`, problem: Unable to provision, see the following errors: 1) Problem parsing object at prefix[druid.lookup]: Cannot construct instance of `LookupListeningAnnouncerConfig`, problem: Unable to provision, see the following errors: 1) [Guice/ErrorInCustomProvider]: IllegalStateException: This is a proxy used to support circular references.

I see, thanks for the clarification!

kfaraz · 2025-11-28T07:10:42Z

services/src/main/java/org/apache/druid/cli/PeonLoadSpecHolder.java

+  private Provider<Task> taskProvider;
+
+  @Inject
+  public PeonLoadSpecHolder(


Similar to PeonTaskHolder, this constructor should not need @Inject. Use a @Provides method in CliPeon instead.

This is intentional - please see #18732 (comment)

server/src/test/java/org/apache/druid/server/metrics/GroupByStatsMonitorTest.java

kfaraz

Only pending comment is moving the mapOfDimensions method to TaskHolder, but that is not a blocker for this PR.

kfaraz · 2025-12-01T04:10:55Z

services/src/test/java/org/apache/druid/cli/CliPeonTest.java

+  }
+
+  @Test
+  public void testTaskWithMonitorsAndMetricsSpecDoNotCauseCyclicDependency() throws IOException


thanks for adding these tests!

abhishekrb19 · 2025-12-01T17:27:36Z

Thanks, @kfaraz!

Move lookup and broadcast datasource load spec configs to its own cla…

64b8b73

…ss to avoid confusion.

abhishekrb19 requested a review from kfaraz November 10, 2025 05:07

No final in strict compilation mode

eb3feb3

github-advanced-security bot found potential problems Nov 10, 2025

View reviewed changes

server/src/main/java/org/apache/druid/query/lookup/LookupListeningAnnouncerConfig.java Fixed Show fixed Hide fixed

Add indexing-logs to CI artifact

51c5f85

github-actions bot added the GHA label Nov 10, 2025

kfaraz reviewed Nov 10, 2025

View reviewed changes

abhishekrb19 added 3 commits November 12, 2025 23:27

Consolidate into one TaskPropertiesHolder

ccaefec

Works with named injections

2b08b0d

Test works!

ca5e6ae

abhishekrb19 added 6 commits November 24, 2025 14:21

Different approach

eb80524

Cleanup MetricsModule

32e672e

Merge branch 'master' into refactor_ds_lookup_holder

a434eeb

Revert changes to EmbeddedKafkaSupervisorTest

5033413

Revert logging changes

f44be2f

Better exception

2093cd1

github-actions bot added the Area - Ingestion label Nov 24, 2025

Cleanup and add LoadSpecHolder

6fd60d6

abhishekrb19 force-pushed the refactor_ds_lookup_holder branch from 98ff33e to 6fd60d6 Compare November 24, 2025 23:52

github-advanced-security bot found potential problems Nov 25, 2025

View reviewed changes

abhishekrb19 added 3 commits November 24, 2025 16:55

Try ImplementedBy to avoid explicit bindings everywhere

58185c0

Revert "Try ImplementedBy to avoid explicit bindings everywhere"

519d536

This reverts commit 58185c0.

Add common DefaultServerHolderModule

0ec0380

abhishekrb19 force-pushed the refactor_ds_lookup_holder branch 2 times, most recently from 9faa21f to 9513f0c Compare November 25, 2025 01:42

Cleanup CliPeonTest and rename

89e3418

abhishekrb19 force-pushed the refactor_ds_lookup_holder branch from 9513f0c to 89e3418 Compare November 25, 2025 01:47

abhishekrb19 added 4 commits November 24, 2025 19:14

Fix test

b04d09c

Fix quidem test module init

98979dc

Rename to mapOfTaskHolderDimensions()

efa0b5e

Add test for default non-peon servers

b81a9b6

github-advanced-security bot found potential problems Nov 26, 2025

View reviewed changes

abhishekrb19 added 2 commits November 26, 2025 08:22

Return null from NoopTaskHolder to avoid callers from doing instanceo…

91acfd3

…f checks, etc

Merge branch 'master' into refactor_ds_lookup_holder

014dac2

abhishekrb19 requested a review from kfaraz November 27, 2025 05:44

kfaraz reviewed Nov 28, 2025

View reviewed changes

abhishekrb19 added 2 commits November 29, 2025 17:50

Review suggestions & more tests

9ed233a

Merge branch 'master' into refactor_ds_lookup_holder

a810414

github-actions bot added the Area - Streaming Ingestion label Nov 30, 2025

kfaraz approved these changes Dec 1, 2025

View reviewed changes

abhishekrb19 merged commit b3be269 into master Dec 1, 2025
111 of 113 checks passed

abhishekrb19 deleted the refactor_ds_lookup_holder branch December 1, 2025 17:29

@@ -56,10 +56,10 @@
                     "Cannot specify both `lookupTier` and `lookupTierIsDatasource`"
                 );
-                final String lookupTier = lookupTierIsDatasource ? taskHolder.getDataSource() : this.lookupTier;
+                final String effectiveLookupTier = lookupTierIsDatasource ? taskHolder.getDataSource() : this.lookupTier;
                 return Preconditions.checkNotNull(
-                    lookupTier == null ? DEFAULT_TIER : StringUtils.emptyToNullNonDruidDataString(lookupTier),
+                    effectiveLookupTier == null ? DEFAULT_TIER : StringUtils.emptyToNullNonDruidDataString(effectiveLookupTier),
                     "Cannot have empty lookup tier from %s",
                     lookupTierIsDatasource ? "bound value" : LookupModule.PROPERTY_BASE
                 );

		// CliPeon.runTask and TaskHolder.getDataSource()/TaskHolder.getTaskId(). The reason for this is unclear
		// but by injecting TaskPropertiesHolder early this cycle is avoided.

Refactor: Rename DataSourceTaskIdHolder and extract load spec holder to its own class #18732

Refactor: Rename DataSourceTaskIdHolder and extract load spec holder to its own class #18732

Uh oh!

Conversation

abhishekrb19 commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kfaraz left a comment

Choose a reason for hiding this comment

Uh oh!

abhishekrb19 commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kfaraz commented Nov 10, 2025

Uh oh!

abhishekrb19 commented Nov 20, 2025

Uh oh!

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

Uh oh!

Uh oh!

abhishekrb19 commented Nov 20, 2025

Uh oh!

Check notice

Uh oh!

Uh oh!

Copilot Autofix

Uh oh!

Check notice

Uh oh!

Copilot Autofix

kfaraz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhishekrb19 Nov 30, 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!

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!

kfaraz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhishekrb19 commented Dec 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

abhishekrb19 commented Nov 10, 2025 •

edited

Loading

abhishekrb19 commented Nov 10, 2025 •

edited

Loading

kfaraz commented Nov 20, 2025 •

edited

Loading

abhishekrb19 Nov 30, 2025 •

edited

Loading