fix: dedicated ephemeral storage loading pool for tasks by clintropolis · Pull Request #19658 · apache/druid

clintropolis · 2026-07-06T21:33:05Z

Description

Fixes an issue originally noticed in #19535 (review) though actually not from that PR, where the per task storage loading pool was not shutdown after task completion, resulting in leaked pools when used in an indexer process. The fix injects a new 'ephemeral' pool to re-use for tasks tied to the lifecycle.

FrankChen021

I have reviewed the code for correctness, edge cases, concurrency, and integration risks; no issues found.

Reviewed 7 of 7 changed files.

This is an automated review by Codex GPT-5.5

FrankChen021

I have reviewed the code for correctness, edge cases, concurrency, and integration risks; no issues found.

Reviewed 7 of 7 changed files.

This is an automated review by Codex GPT-5.5

capistrant

left non-diff comment on potential logger config issue + one non-blocking suggestion.

capistrant · 2026-07-08T15:16:00Z

unrelated to the PR, but is this supposed to be new Logger(StorageLoadingThreadPool.class);?

capistrant · 2026-07-08T15:23:11Z

-            )
-        );
-      }
+  private static ListeningExecutorService createOnDemandLoadingExecutor(final SegmentLoaderConfig config)


Consider value of adding a "purpose" parameter that can enrich the logs based on if it is the ephemeral create or the create from config. would help avoid confusion for an operator who doesn't have process level virtual storage flipped on but still sees logs for it due to ephemeral create. not blocking

gianm · 2026-07-07T03:34:18Z

+   * this is fine for short-lived test JVMs.
+   */
+  @VisibleForTesting
+  public SegmentCacheManagerFactory(


I believe that @VisibleForTesting is more for things that are used in both production and tests, where the visibility is broader than necessary for production, due to the requirements of the tests. This constructor seems like something that is test-only. I would suggest making it a static factory method instead, like SegmentCacheManagerFactory.createWithOwnedPool.

gianm · 2026-07-08T19:38:42Z

+   *
+   * @see org.apache.druid.guice.annotations.EphemeralStorageLoading
+   */
+  public static StorageLoadingThreadPool createForEphemeral(final SegmentLoaderConfig config)


IMO it would be cleaner to remove this method, add withVirtualStorage to SegmentLoaderConfig, and have the Guice module call createFromConfig with a config adjusted to have virtualStorage = true. (Alternatively: add copy to SegmentLoaderConfig and then call setVirtualStorage on the copy.)

This will simplify the logic in StorageLoadingThreadPool and make it more obvious what is happening. IMO, it is not obvious what "createForEphemeral" means, but it would be obvious what createFromConfig(config.withVirtualStorage(true)) means.

gianm · 2026-07-08T20:48:14Z

      IndexIO indexIO,
-      @Json ObjectMapper mapper
+      @Json ObjectMapper mapper,
+      @EphemeralStorageLoading StorageLoadingThreadPool loadingThreadPool


Hmm. The SegmentCacheManagerFactory is injected into DruidInputSource, which will cause the pool to be created anywhere that a DruidInputSource is instantiated. If used in task specs, that would be Overlord, MM, Peon, Indexer, and possibly even Broker. I don't think we want the EphemeralStorageLoading pool to exist in all these places. It won't even be used by the DruidInputSource anyway, since manufacturate is called with virtualStorage = false.

Maybe this can be fixed by injecting IndexIO and @Json ObjectMapper directly into DruidInputSource, rather than injecting SegmentCacheManagerFactory?

FrankChen021

Severity	Findings
P0	0
P1	0
P2	3
P3	0
Total	3

Severity	Findings
P0	0
P1	0
P2	3
P3	0
Total	3

Reviewed 26 of 26 changed files.

Found three lifecycle and compatibility issues: the shared pool can stop before active tasks, two virtual-storage fetch settings are lost when copying config, and a public constructor was removed.

This is an automated review by Codex GPT-5.6-Sol

FrankChen021 · 2026-07-16T12:17:38Z

        storageLocations,
        loaderConfig,
-        loadingThreadPool,
+        virtualStorage ? loadingThreadPoolProvider.get() : StorageLoadingThreadPool.none(),


[P2] Keep the pool alive until task shutdown completes

On the Indexer, this provider is first resolved from a running task, after the NORMAL-scoped WorkerTaskManager has been registered. Lifecycle shutdown reverses NORMAL registration order, so StorageLoadingThreadPool.stop() runs first and calls shutdownNow() before WorkerTaskManager invokes ThreadingTaskRunner.stop() to gracefully stop active tasks. Any task still doing or scheduling on-demand loads is interrupted or rejected during cleanup. Register this pool in an earlier lifecycle stage, or otherwise before the worker manager, while retaining lazy provisioning.

i just removed this from the lifecycle and marked it lazy singleton instead, which I think should be ok as I would expect nothing to be running on the pool by the time of shutdown since all of the threads running things that could be using it will have been stopped by the time of process exit.

FrankChen021 · 2026-07-16T12:17:38Z

+    this.virtualStorageIsEphemeral = other.virtualStorageIsEphemeral;
+    this.virtualStorageMetadataReservationEstimate = other.virtualStorageMetadataReservationEstimate;
+    this.virtualStoragePartialDownloadsEnabled = other.virtualStoragePartialDownloadsEnabled;
+    this.combinedMaxSize = other.combinedMaxSize;


[P2] Preserve all virtual-storage fetch settings in the copy

The new copy constructor ends here without copying virtualStorageCoalesceGapBytes or virtualStorageMaxFetchRunBytes, even though withVirtualStorage promises every other setting is preserved. A non-default configured value is therefore silently reset to the class default in the returned config. Copy both fields and extend the test with non-default values.

+1, virtualStorageCoalesceGapBytes and virtualStorageMaxFetchRunBytes are apparently missing.

FrankChen021 · 2026-07-16T12:17:38Z

+   * for tests and manual construction where that shared pool is not available via injection; the created pool is not
+   * lifecycle-managed, which is fine for short-lived JVMs.
+   */
+  public static SegmentCacheManagerFactory createWithOwnedPool(IndexIO indexIO, ObjectMapper mapper)


[P2] Retain the existing two-argument constructor

The follow-up replaces the existing public SegmentCacheManagerFactory(IndexIO, ObjectMapper) constructor with this static factory and updates only repository call sites. External extensions or tests compiled against that constructor will now fail to link or compile on upgrade, despite equivalent behavior remaining available. Keep a delegating, possibly deprecated, two-argument constructor unless this compatibility break is intentional.

IMO it's OK (and preferred) to remove the 2-arg constructor.

FrankChen021

I re-reviewed the follow-up changes and discussion. The config-copy issue is fixed, the constructor removal is intentional, and the shared pool now remains available through task shutdown. No remaining correctness issues found.

Reviewed 26 of 26 changed files.

This is an automated review by Codex GPT-5.6-Sol

fix: dedicated ephemeral storage loading pool for tasks

c4cf1b4

github-actions Bot added the Area - Ingestion label Jul 6, 2026

FrankChen021 reviewed Jul 7, 2026

View reviewed changes

capistrant approved these changes Jul 8, 2026

View reviewed changes

gianm reviewed Jul 8, 2026

View reviewed changes

review comments

b479e1e

github-actions Bot added Area - Batch Ingestion Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 labels Jul 15, 2026

clintropolis added 2 commits July 15, 2026 16:06

Merge remote-tracking branch 'upstream/master' into ephemeral-load-pool

6960ed5

oops

a597ca1

FrankChen021 reviewed Jul 16, 2026

View reviewed changes

capistrant self-requested a review July 16, 2026 15:20

lazy singleton instead of lifecycle

f0c6b3c

FrankChen021 reviewed Jul 18, 2026

View reviewed changes

Uh oh!

Conversation

clintropolis commented Jul 6, 2026

Description

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

Uh oh!

capistrant 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!

FrankChen021 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!

FrankChen021 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants