You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "clintropolis (via GitHub)" <gi...@apache.org> on 2026/07/23 18:52:34 UTC
[PR] feat: support partial loads for tasks (druid)
clintropolis opened a new pull request, #19739:
URL: https://github.com/apache/druid/pull/19739
### Description
This PR enables tasks to set SegmentLoaderConfig#virtualStoragePartialDownloadsEnabled to true (default false) via runtime property `druid.indexer.task.virtualStoragePartialDownloadsEnabled` or setting `virtualStoragePartialDownloadsEnabled` on MSQ query context.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feat: support partial loads for tasks (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #19739:
URL: https://github.com/apache/druid/pull/19739#discussion_r3645237398
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/SegmentCacheManagerFactory.java:
##########
@@ -105,7 +114,8 @@ public SegmentCacheManager manufacturate(File storageDir, Long maxSize, boolean
new SegmentLoaderConfig()
.setLocations(Collections.singletonList(locationConfig))
.setVirtualStorage(virtualStorage)
- .setVirtualStorageIsEphemeral(virtualStorage);
+ .setVirtualStorageIsEphemeral(virtualStorage)
+ .setVirtualStoragePartialDownloadsEnabled(partialDownloadsEnabled);
Review Comment:
[P2] Preserve the configured partial-download tuning
This fresh `SegmentLoaderConfig` only copies the enable flag, so per-task caches silently discard the configured metadata reservation and range-fetch/coalescing limits from the injected `druid.segmentCache` configuration. In particular, a V10 header larger than the hard-coded 16 MiB default still fails while directing the operator to raise `druid.segmentCache.virtualStorageMetadataReservationEstimate`, because that setting can never reach this cache. Derive this per-task config from the injected loader config before replacing the location and ephemeral flags.
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/TaskToolboxFactory.java:
##########
@@ -247,7 +247,15 @@ public TaskToolbox build(TaskConfig config, Task task)
.queryProcessingPool(queryProcessingPool)
.joinableFactory(joinableFactory)
.monitorSchedulerProvider(monitorSchedulerProvider)
- .segmentCacheManager(segmentCacheManagerFactory.manufacturate(taskWorkDir, null, true))
+ .segmentCacheManager(segmentCacheManagerFactory.manufacturate(
+ taskWorkDir,
+ null,
+ true,
+ task.getContextValue(
Review Comment:
[P2] Parse the query-context boolean before unboxing
This key is also exposed as an MSQ query-context parameter, whose `QueryContext.getBoolean` contract accepts string booleans. If a request supplies `"false"`, that value is copied into the worker task context, but this raw generic lookup returns a `String` and unboxing it as `boolean` throws `ClassCastException` while constructing the toolbox, before `IndexerWorkerContext` can use the proper parser. Parse the value with `QueryContext` or the existing boolean conversion utility.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feat: support partial loads for tasks (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #19739:
URL: https://github.com/apache/druid/pull/19739#discussion_r3652472127
##########
server/src/main/java/org/apache/druid/segment/loading/SegmentLoaderConfig.java:
##########
@@ -283,24 +286,40 @@ public SegmentLoaderConfig setVirtualStoragePartialDownloadsEnabled(boolean enab
}
/**
- * Sets {@link #virtualStorage}.
+ * Sets {@link #virtualStorage}. When enabling it, settings that only apply to classic on-disk historical caches are
+ * reset to values safe for on-demand virtual-storage caches, so that a per-task config derived from a node's
+ * {@code druid.segmentCache} config (via {@link #withVirtualStorage}) does not inherit historical-only behavior. This
+ * runs only for programmatic enabling; a config with {@code virtualStorage} deserialized straight from JSON is left
+ * as the operator specified it.
*/
public SegmentLoaderConfig setVirtualStorage(boolean virtualStorage)
{
this.virtualStorage = virtualStorage;
+ if (virtualStorage) {
+ // On-demand virtual-storage caches never bootstrap or lazy-load at startup, never warm the OS page cache, must
+ // not share a persistent info directory (each task cache has its own ephemeral location), and must delete files
+ // as soon as all holds are released.
+ this.lazyLoadOnStart = false;
Review Comment:
[P2] Avoid sanitizing operator config through the Jackson setter
`setVirtualStorage` is also Jackson's bean mutator for the `virtualStorage` property, so `JsonConfigProvider` calls this branch while loading `druid.segmentCache`. Consequently operator config is not left untouched as promised: for example, `virtualStorage=true` with `lazyLoadOnStart=true` can have the latter applied first and then silently reset to false, changing bootstrap behavior on existing virtual-storage Historicals. Direct JSON also becomes property-order dependent. Keep this sanitization in `withVirtualStorage(true)` or a dedicated per-task derivation method, leave the bean setter as a simple assignment, and add a deserialization regression test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feat: support partial loads for tasks (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #19739:
URL: https://github.com/apache/druid/pull/19739#discussion_r3665455976
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/SegmentCacheManagerFactory.java:
##########
@@ -88,24 +96,39 @@ public static SegmentCacheManagerFactory createWithOwnedPool(IndexIO indexIO, Ob
* {@link EphemeralStorageLoading} loading pool (shared across all per-task caches and stopped by the lifecycle)
* rather than creating its own.
*
- * @param storageDir storage location
- * @param maxSize size limit, or null for no limit
- * @param virtualStorage whether to configure the cache manager in ephemeral virtual storage mode. In this mode,
- * loading is triggered by {@link SegmentCacheManager#acquireSegment(DataSegment, AcquireMode)},
- * and segment files are deleted as soon as all holds are closed.
+ * @param storageDir storage location
+ * @param maxSize size limit, or null for no limit
+ * @param virtualStorage whether to configure the cache manager in ephemeral virtual storage mode. In this
+ * mode, loading is triggered by
+ * {@link SegmentCacheManager#acquireSegment(DataSegment, AcquireMode)}, and segment
+ * files are deleted as soon as all holds are closed.
+ * @param partialDownloadsEnabled when true (and {@code virtualStorage} is true), partial-eligible segments are read
+ * via on-demand column downloads rather than downloaded in full up front. Has no
+ * effect when {@code virtualStorage} is false.
*/
- public SegmentCacheManager manufacturate(File storageDir, Long maxSize, boolean virtualStorage)
+ public SegmentCacheManager manufacturate(
+ File storageDir,
+ Long maxSize,
+ boolean virtualStorage,
+ boolean partialDownloadsEnabled
+ )
{
final StorageLocationConfig locationConfig = new StorageLocationConfig(
storageDir,
maxSize != null ? maxSize : Long.MAX_VALUE,
null
);
+ // For virtual storage, derive an ephemeral virtual-storage cache config from the injected node config: this keeps
+ // its virtual-storage tuning while dropping the classic on-disk-cache settings that don't apply to a per-task
+ // cache. Otherwise, start from the node config as-is. Then set the per-task location and mode flags.
final SegmentLoaderConfig loaderConfig =
- new SegmentLoaderConfig()
- .setLocations(Collections.singletonList(locationConfig))
- .setVirtualStorage(virtualStorage)
- .setVirtualStorageIsEphemeral(virtualStorage);
+ (virtualStorage ? segmentLoaderConfig.toEphemeralVirtualStorage() : segmentLoaderConfig)
Review Comment:
[P2] Keep node cache settings out of nonvirtual input caches
When `virtualStorage` is false, the production caller is `DruidInputSource`, but this branch now clones the process-wide `druid.segmentCache` configuration instead of the fresh defaults used before this PR. With `numThreadsToLoadSegmentsIntoPageCacheOnDownload > 0`, every reader creates and starts its own fixed executor; the input entity only drops segments and never calls `shutdown()`, so a persistent Indexer leaks a pool per reader/task. Inheriting `deleteOnRemove=false` also leaves downloaded input segments behind. Derive this branch from defaults or reset these transient cache settings; only virtual managers need the injected tuning.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feat: support partial loads for tasks (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #19739:
URL: https://github.com/apache/druid/pull/19739#discussion_r3642737293
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/config/TaskConfig.java:
##########
@@ -45,6 +45,7 @@ public class TaskConfig implements TaskDirectory
private static final Period DEFAULT_GRACEFUL_SHUTDOWN_TIMEOUT = new Period("PT5M");
private static final boolean DEFAULT_STORE_EMPTY_COLUMNS = true;
private static final long DEFAULT_TMP_STORAGE_BYTES_PER_TASK = -1;
+ private static final boolean DEFAULT_VIRTUAL_STORAGE_PARTIAL_DOWNLOADS_ENABLED = false;
Review Comment:
IMO this should default to `true`, because it already requires v10 segments, which are opt-in. If you opt-in to v10 segments then you should get all the cool v10 stuff automatically.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feat: support partial loads for tasks (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #19739:
URL: https://github.com/apache/druid/pull/19739#discussion_r3650318921
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/SegmentCacheManagerFactory.java:
##########
@@ -88,24 +97,33 @@ public static SegmentCacheManagerFactory createWithOwnedPool(IndexIO indexIO, Ob
* {@link EphemeralStorageLoading} loading pool (shared across all per-task caches and stopped by the lifecycle)
* rather than creating its own.
*
- * @param storageDir storage location
- * @param maxSize size limit, or null for no limit
- * @param virtualStorage whether to configure the cache manager in ephemeral virtual storage mode. In this mode,
- * loading is triggered by {@link SegmentCacheManager#acquireSegment(DataSegment, AcquireMode)},
- * and segment files are deleted as soon as all holds are closed.
+ * @param storageDir storage location
+ * @param maxSize size limit, or null for no limit
+ * @param virtualStorage whether to configure the cache manager in ephemeral virtual storage mode. In this
+ * mode, loading is triggered by
+ * {@link SegmentCacheManager#acquireSegment(DataSegment, AcquireMode)}, and segment
+ * files are deleted as soon as all holds are closed.
+ * @param partialDownloadsEnabled when true (and {@code virtualStorage} is true), partial-eligible segments are read
+ * via on-demand column downloads rather than downloaded in full up front. Has no
+ * effect when {@code virtualStorage} is false.
*/
- public SegmentCacheManager manufacturate(File storageDir, Long maxSize, boolean virtualStorage)
+ public SegmentCacheManager manufacturate(
+ File storageDir,
+ Long maxSize,
+ boolean virtualStorage,
+ boolean partialDownloadsEnabled
+ )
{
final StorageLocationConfig locationConfig = new StorageLocationConfig(
storageDir,
maxSize != null ? maxSize : Long.MAX_VALUE,
null
);
final SegmentLoaderConfig loaderConfig =
- new SegmentLoaderConfig()
+ segmentLoaderConfig.withVirtualStorage(virtualStorage)
Review Comment:
[P2] Preserve only compatible virtual-storage settings
Copying the entire injected `SegmentLoaderConfig` also inherits `numThreadsToLoadSegmentsIntoPageCacheOnDownload` and `numThreadsToLoadSegmentsIntoPageCacheOnBootstrap`. If either valid operator setting is nonzero, every `virtualStorage=true` task cache now throws during `SegmentLocalCacheManager` construction because virtual storage rejects page-cache loading threads. Previously task caches started from defaults. Copy only the intended virtual-storage tuning fields or explicitly reset the incompatible settings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feat: support partial loads for tasks (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis merged PR #19739:
URL: https://github.com/apache/druid/pull/19739
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org