FrankChen021 commented on code in PR #19658:
URL: https://github.com/apache/druid/pull/19658#discussion_r3595313300
##########
server/src/main/java/org/apache/druid/segment/loading/SegmentLoaderConfig.java:
##########
@@ -137,6 +137,32 @@ public class SegmentLoaderConfig
private long combinedMaxSize = 0;
+ public SegmentLoaderConfig()
+ {
+ }
+
+ private SegmentLoaderConfig(SegmentLoaderConfig other)
+ {
+ this.locations = other.locations;
+ this.lazyLoadOnStart = other.lazyLoadOnStart;
+ this.deleteOnRemove = other.deleteOnRemove;
+ this.dropSegmentDelayMillis = other.dropSegmentDelayMillis;
+ this.announceIntervalMillis = other.announceIntervalMillis;
+ this.numLoadingThreads = other.numLoadingThreads;
+ this.numBootstrapThreads = other.numBootstrapThreads;
+ this.numThreadsToLoadSegmentsIntoPageCacheOnDownload =
other.numThreadsToLoadSegmentsIntoPageCacheOnDownload;
+ this.numThreadsToLoadSegmentsIntoPageCacheOnBootstrap =
other.numThreadsToLoadSegmentsIntoPageCacheOnBootstrap;
+ this.infoDir = other.infoDir;
+ this.statusQueueMaxSize = other.statusQueueMaxSize;
+ this.virtualStorage = other.virtualStorage;
+ this.virtualStorageLoadThreads = other.virtualStorageLoadThreads;
+ this.virtualStorageUseVirtualThreads =
other.virtualStorageUseVirtualThreads;
+ this.virtualStorageIsEphemeral = other.virtualStorageIsEphemeral;
+ this.virtualStorageMetadataReservationEstimate =
other.virtualStorageMetadataReservationEstimate;
+ this.virtualStoragePartialDownloadsEnabled =
other.virtualStoragePartialDownloadsEnabled;
+ this.combinedMaxSize = other.combinedMaxSize;
Review Comment:
[P2] Preserve all virtual-storage fetch settings in the copy
The new copy constructor ends here without copying
`virtualStorageCoalesceGapBytes` or `virtualStorageMaxFetchRunBytes`, even
though `withVirtualStorage` promises every other setting is preserved. A
non-default configured value is therefore silently reset to the class default
in the returned config. Copy both fields and extend the test with non-default
values.
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/SegmentCacheManagerFactory.java:
##########
@@ -77,11 +107,10 @@ public SegmentCacheManager manufacturate(File storageDir,
Long maxSize, boolean
.setVirtualStorage(virtualStorage)
.setVirtualStorageIsEphemeral(virtualStorage);
final List<StorageLocation> storageLocations =
loaderConfig.toStorageLocations();
- final StorageLoadingThreadPool loadingThreadPool =
StorageLoadingThreadPool.createFromConfig(loaderConfig);
return new SegmentLocalCacheManager(
storageLocations,
loaderConfig,
- loadingThreadPool,
+ virtualStorage ? loadingThreadPoolProvider.get() :
StorageLoadingThreadPool.none(),
Review Comment:
[P2] Keep the pool alive until task shutdown completes
On the Indexer, this provider is first resolved from a running task, after
the NORMAL-scoped `WorkerTaskManager` has been registered. Lifecycle shutdown
reverses NORMAL registration order, so `StorageLoadingThreadPool.stop()` runs
first and calls `shutdownNow()` before `WorkerTaskManager` invokes
`ThreadingTaskRunner.stop()` to gracefully stop active tasks. Any task still
doing or scheduling on-demand loads is interrupted or rejected during cleanup.
Register this pool in an earlier lifecycle stage, or otherwise before the
worker manager, while retaining lazy provisioning.
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/SegmentCacheManagerFactory.java:
##########
@@ -43,26 +47,52 @@ public class SegmentCacheManagerFactory
{
private final IndexIO indexIO;
private final ObjectMapper jsonMapper;
+ /**
+ * Resolved lazily (only on the {@code virtualStorage=true} path of {@link
#manufacturate}) so that injecting this
+ * factory into a component that only builds non-virtual caches (e.g. {@code
DruidInputSource}) does not force the
+ * process-wide {@link EphemeralStorageLoading} pool to be created on
processes that never do on-demand loading
+ * (Overlord, MiddleManager).
+ */
+ private final Provider<StorageLoadingThreadPool> loadingThreadPoolProvider;
@Inject
public SegmentCacheManagerFactory(
IndexIO indexIO,
- @Json ObjectMapper mapper
+ @Json ObjectMapper mapper,
+ @EphemeralStorageLoading Provider<StorageLoadingThreadPool>
loadingThreadPoolProvider
)
{
this.indexIO = indexIO;
this.jsonMapper = mapper;
+ this.loadingThreadPoolProvider = loadingThreadPoolProvider;
+ }
+
+ /**
+ * Creates a factory whose {@code virtualStorage=true} managers get their
own private loading pool of default size,
+ * instead of the process-wide, lifecycle-managed {@link
EphemeralStorageLoading} pool that Guice injects. Intended
+ * for tests and manual construction where that shared pool is not available
via injection; the created pool is not
+ * lifecycle-managed, which is fine for short-lived JVMs.
+ */
+ public static SegmentCacheManagerFactory createWithOwnedPool(IndexIO
indexIO, ObjectMapper mapper)
Review Comment:
[P2] Retain the existing two-argument constructor
The follow-up replaces the existing public
`SegmentCacheManagerFactory(IndexIO, ObjectMapper)` constructor with this
static factory and updates only repository call sites. External extensions or
tests compiled against that constructor will now fail to link or compile on
upgrade, despite equivalent behavior remaining available. Keep a delegating,
possibly deprecated, two-argument constructor unless this compatibility break
is intentional.
--
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]
