bsyk opened a new issue, #19833:
URL: https://github.com/apache/druid/issues/19833
**Affects:** 37.0.0 (verified). Introduced with virtual storage in #18176
(milestone 35.0.0).
**Type:** Bug / configuration gap
---
## Summary
When an indexing task (e.g. auto-compaction) runs with virtual storage
enabled, its on-demand
segment download pool is sized from
`SegmentLoaderConfig.virtualStorageLoadThreads`. That value can
never be configured for tasks: `SegmentCacheManagerFactory.manufacturate()`
constructs a fresh
`SegmentLoaderConfig` with `new`, bypassing property binding, and the class
exposes no setter for
the field. The pool is therefore always `2 * availableProcessors`, and the
only way to influence it
is `-XX:ActiveProcessorCount`, which also resizes GC, JIT and `ForkJoinPool`
threads.
This matters most for compaction, which is the workload that downloads the
most segments. In our
production cluster a single compaction task pulls **1,807 segments in 53
minutes** at 4 threads,
doing no computation for that entire period.
## Reproduction
Druid 37.0.0. Run any compaction task on a cluster where indexing tasks use
virtual storage, with
`-XX:ActiveProcessorCount=2` in the task's `druid.indexer.runner.javaOpts`.
The peon logs:
```
CliPeon - * druid.segmentCache.virtualStorageLoadThreads: 8
SegmentLocalCacheManager - Using virtual storage mode - on demand load
threads: [4].
```
The property is received and ignored. We tested both documented delivery
mechanisms, as two ad-hoc
compaction tasks on the same datasource minutes apart:
| task | context | resulting pool |
|---|---|---|
| A |
`druid.indexer.fork.property.druid.segmentCache.virtualStorageLoadThreads=8`,
`-XX:ActiveProcessorCount=2` | `[4]` |
| B | no property at all, `-XX:ActiveProcessorCount=4` | `[8]` |
`ForkingTaskRunner` converts a `fork.property` context key into
`command.addSystemProperty(...)`, so
both mechanisms deliver a `-D` system property to the peon — the same
destination, both ignored.
That the pool really is the one fetching segments for compaction is
confirmed by thread names: all
segment pulls run on `VirtualStorageOnDemandLoadingThread-N`, and that
thread factory exists only in
this pool.
## Cause
`indexing-service/.../SegmentCacheManagerFactory.java`:
```java
public SegmentCacheManager manufacturate(File storageDir, boolean
virtualStorage)
{
final SegmentLoaderConfig loaderConfig =
new SegmentLoaderConfig()
.setLocations(Collections.singletonList(new
StorageLocationConfig(storageDir, null, null)))
.setVirtualStorage(virtualStorage, virtualStorage);
...
}
```
`new SegmentLoaderConfig()` is never populated by Jackson from the
`druid.segmentCache.*` properties,
and `SegmentLoaderConfig` has setters only for `locations` and
`virtualStorage` — not for
`virtualStorageLoadThreads`. The field keeps its initializer:
```java
@JsonProperty("virtualStorageLoadThreads")
private int virtualStorageLoadThreads = 2 *
runtimeInfo.getAvailableProcessors();
```
`SegmentLocalCacheManager` then builds the pool from it:
```java
virtualStorageLoadOnDemandExec = MoreExecutors.listeningDecorator(
Executors.newFixedThreadPool(config.getVirtualStorageLoadThreads(),
...));
```
The setting works as documented on Historicals, where `SegmentLoaderConfig`
*is* bound from
properties. The gap is specific to the indexing-task path.
## Impact
- The one tunable for download concurrency is unavailable exactly where
segment counts are highest.
- The available workaround, raising `-XX:ActiveProcessorCount`, is a blunt
instrument: it changes GC
thread count, JIT compiler threads, `ForkJoinPool.commonPool` and Druid's
own
`availableProcessors`-derived pools, so download concurrency cannot be
tuned independently of CPU
allocation.
- Silent: the property is accepted, echoed in the peon's config, and
ignored. Nothing warns.
## Suggested fix
Either would do:
1. Inject the bound `SegmentLoaderConfig` into `SegmentCacheManagerFactory`
and derive from it
(`.setLocations(...)`/`.setVirtualStorage(...)` on a copy), so
`druid.segmentCache.*` applies to
tasks as it does to Historicals.
2. Add a `setVirtualStorageLoadThreads(int)` setter and have the factory
read the value from task
context or peon properties.
Option 1 seems more consistent with how the rest of `druid.segmentCache.*`
behaves.
## Note for 38
We understand the default multiplier becomes `4 * availableProcessors` in
38, which raises the pool
without configuration. That reduces the urgency but does not close the gap —
the value still cannot
be tuned for tasks.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
