fix: dedicated ephemeral storage loading pool for tasks#19658
fix: dedicated ephemeral storage loading pool for tasks#19658clintropolis wants to merge 1 commit into
Conversation
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
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
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
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 a comment
capistrant
left a comment
There was a problem hiding this comment.
left non-diff comment on potential logger config issue + one non-blocking suggestion.
| */ | ||
| public class StorageLoadingThreadPool | ||
| { | ||
| private static final Logger log = new Logger(StorageNodeModule.class); |
There was a problem hiding this comment.
unrelated to the PR, but is this supposed to be new Logger(StorageLoadingThreadPool.class);?
| ) | ||
| ); | ||
| } | ||
| private static ListeningExecutorService createOnDemandLoadingExecutor(final SegmentLoaderConfig config) |
There was a problem hiding this comment.
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
| * this is fine for short-lived test JVMs. | ||
| */ | ||
| @VisibleForTesting | ||
| public SegmentCacheManagerFactory( |
There was a problem hiding this comment.
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.
| * | ||
| * @see org.apache.druid.guice.annotations.EphemeralStorageLoading | ||
| */ | ||
| public static StorageLoadingThreadPool createForEphemeral(final SegmentLoaderConfig config) |
There was a problem hiding this comment.
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.
| IndexIO indexIO, | ||
| @Json ObjectMapper mapper | ||
| @Json ObjectMapper mapper, | ||
| @EphemeralStorageLoading StorageLoadingThreadPool loadingThreadPool |
There was a problem hiding this comment.
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?
4 participants
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.