capistrant commented on code in PR #19658:
URL: https://github.com/apache/druid/pull/19658#discussion_r3545152423
##########
server/src/main/java/org/apache/druid/segment/loading/StorageLoadingThreadPool.java:
##########
Review Comment:
unrelated to the PR, but is this supposed to be `new
Logger(StorageLoadingThreadPool.class);`?
##########
server/src/main/java/org/apache/druid/segment/loading/StorageLoadingThreadPool.java:
##########
@@ -63,49 +63,59 @@ public StorageLoadingThreadPool(
public static StorageLoadingThreadPool createFromConfig(final
SegmentLoaderConfig config)
{
- final ListeningExecutorService exec;
+ return new StorageLoadingThreadPool(config.isVirtualStorage() ?
createOnDemandLoadingExecutor(config) : null);
+ }
+
+ /**
+ * Build a pool configured for virtual-storage on-demand loading
<b>regardless of</b>
+ * {@link SegmentLoaderConfig#isVirtualStorage()}. Used for the process-wide
loading pool shared by ephemeral,
+ * per-task segment caches, whose host process may not itself run in
virtual-storage mode. The executor is created
+ * eagerly but spawns no threads until work is submitted, so an unused pool
is cheap.
+ *
+ * @see org.apache.druid.guice.annotations.EphemeralStorageLoading
+ */
+ public static StorageLoadingThreadPool createForEphemeral(final
SegmentLoaderConfig config)
+ {
+ return new StorageLoadingThreadPool(createOnDemandLoadingExecutor(config));
+ }
- if (config.isVirtualStorage()) {
- if (config.getVirtualStorageLoadThreads() <= 0) {
- throw DruidException.forPersona(DruidException.Persona.OPERATOR)
- .ofCategory(DruidException.Category.INVALID_INPUT)
- .build(
- "virtualStorageLoadThreads must be greater
than 0, got [%d]",
- config.getVirtualStorageLoadThreads()
- );
- }
- if (config.isVirtualStorageUseVirtualThreads()) {
- log.info(
- "Using virtual storage mode with virtual threads - max concurrent
on demand loads: [%d].",
- config.getVirtualStorageLoadThreads()
- );
- exec = new PermitBoundedListeningExecutorService(
- MoreExecutors.listeningDecorator(
- Executors.newThreadPerTaskExecutor(
- Thread.ofVirtual()
- .name("VirtualStorageOnDemandLoadingThread-", 0)
- .factory()
- )
- ),
- new Semaphore(config.getVirtualStorageLoadThreads())
- );
- } else {
- log.info(
- "Using virtual storage mode with fixed platform thread pool - on
demand load threads: [%d].",
- config.getVirtualStorageLoadThreads()
- );
- exec = MoreExecutors.listeningDecorator(
- Executors.newFixedThreadPool(
- config.getVirtualStorageLoadThreads(),
-
Execs.makeThreadFactory("VirtualStorageOnDemandLoadingThread-%s")
- )
- );
- }
+ private static ListeningExecutorService createOnDemandLoadingExecutor(final
SegmentLoaderConfig config)
Review 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 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]
