gianm commented on code in PR #19658:
URL: https://github.com/apache/druid/pull/19658#discussion_r3547150331
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/SegmentCacheManagerFactory.java:
##########
@@ -43,26 +46,45 @@ public class SegmentCacheManagerFactory
{
private final IndexIO indexIO;
private final ObjectMapper jsonMapper;
+ private final StorageLoadingThreadPool loadingThreadPool;
@Inject
public SegmentCacheManagerFactory(
IndexIO indexIO,
- @Json ObjectMapper mapper
+ @Json ObjectMapper mapper,
+ @EphemeralStorageLoading StorageLoadingThreadPool loadingThreadPool
Review 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`?
##########
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)
Review 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.
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/SegmentCacheManagerFactory.java:
##########
@@ -43,26 +46,45 @@ public class SegmentCacheManagerFactory
{
private final IndexIO indexIO;
private final ObjectMapper jsonMapper;
+ private final StorageLoadingThreadPool loadingThreadPool;
@Inject
public SegmentCacheManagerFactory(
IndexIO indexIO,
- @Json ObjectMapper mapper
+ @Json ObjectMapper mapper,
+ @EphemeralStorageLoading StorageLoadingThreadPool loadingThreadPool
)
{
this.indexIO = indexIO;
this.jsonMapper = mapper;
+ this.loadingThreadPool = loadingThreadPool;
}
/**
- * Creates a new {@link SegmentCacheManager} backed by a new storage
location in {@code storageDir}, and a new
- * loading thread pool of default size.
+ * Convenience constructor for tests and manual construction. Builds a
private, always-virtual loading pool of default
+ * size rather than sharing the process-wide {@link EphemeralStorageLoading}
pool. The pool is not lifecycle-managed;
+ * this is fine for short-lived test JVMs.
+ */
+ @VisibleForTesting
+ public SegmentCacheManagerFactory(
Review 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`.
--
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]
