renechoi commented on issue #19833: URL: https://github.com/apache/druid/issues/19833#issuecomment-5138085328
On current `master` this looks like it was already closed by #19658 (milestone 38.0.0), which changed more than the default multiplier mentioned in the "Note for 38", so it may be worth re-checking before any work is planned here. The chain that carries `druid.segmentCache.virtualStorageLoadThreads` into a task on `master`: - `CoreInjectorBuilder` installs `StorageNodeModule` for every node role, and that module still does `JsonConfigProvider.bind(binder, "druid.segmentCache", SegmentLoaderConfig.class)`, so the peon's `-D` property is bound there exactly as it is on a Historical. - `SegmentCacheManagerFactory` no longer calls `new SegmentLoaderConfig()`. It takes the bound `SegmentLoaderConfig` by injection, and on the `virtualStorage=true` path derives the per-task config from it via `segmentLoaderConfig.toEphemeralVirtualStorage().toBuilder()`, so operator-set virtual-storage values survive. - `SegmentLoaderConfig#toEphemeralVirtualStorage()` goes through `toBuilder()`, which copies `virtualStorageLoadThreads`. It only overrides the ephemeral-mode fields (`virtualStorage`, `virtualStorageIsEphemeral`, `lazyLoadOnStart`, `infoDir`, the page-cache thread counts, `deleteOnRemove`). - The pool itself is now the process-wide `@EphemeralStorageLoading StorageLoadingThreadPool` provided by `StorageNodeModule#getEphemeralStorageLoadingThreadPool(SegmentLoaderConfig)`, built from that same bound config, and `StorageLoadingThreadPool.createFromConfig` sizes the executor (or the virtual-thread semaphore) from `getVirtualStorageLoadThreads()`. `SegmentCacheManagerFactoryTest#testManufacturateDerivesPerTaskConfigFromInjectedConfig` covers the derivation, and `StorageNodeModuleTest#testEphemeralStorageLoadingThreadPoolIsInjectedAndAvailable` covers the pool binding. Two details on `master` that differ from the report: the default is `Math.max(32, 4 * availableProcessors)`, so there is a floor of 32 that does not move with `-XX:ActiveProcessorCount`, and since `virtualStorageUseVirtualThreads` defaults to true the value is a concurrency permit count rather than a platform-thread pool size. Caveat on what the above is: a read of `master` and of #19658 / #19739, not a run of a 38 build on a cluster. So it is worth confirming with the peon log line you already used before this gets closed. It also leaves 37.0.0 as a separate question, since 37 does have the `new SegmentLoaderConfig()` path you quoted. -- 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]
