You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "clintropolis (via GitHub)" <gi...@apache.org> on 2026/08/19 01:29:53 UTC
[PR] fix: more chill abandoned partial loads and safer eviction of held entries (druid)
clintropolis opened a new pull request, #20073:
URL: https://github.com/apache/druid/pull/20073
### Description
Relaxes cache manager partial segment loading to not throw when pinning reference holds if the caller closed the holds (and so no longer cares to finish mounting and loading the segment), and also fixes a problem where a weak held entry with deferred unmount could be incorrectly unmounted since it was looking up by id without confirming it was the same reference (and not a newly re-added entry).
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: more chill abandoned partial loads and safer deferred eviction of weak held entries (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #20073:
URL: https://github.com/apache/druid/pull/20073#discussion_r3813153133
##########
server/src/main/java/org/apache/druid/segment/loading/StorageLocation.java:
##########
@@ -604,11 +617,14 @@ private Runnable createWeakEntryReleaseRunnable(
weakCacheEntries.computeIfPresent(
weakEntry.cacheEntry.getId(),
(cacheEntryIdentifier, weakCacheEntry) -> {
+ if (weakCacheEntry != weakEntry || weakCacheEntry.isHeld()) {
+ // Someone else's entry, or someone else is still using ours; either way, theirs to clean up.
+ return weakCacheEntry;
+ }
// If we never successfully mounted, go ahead and remove so we don't have a dead entry.
// Furthermore, if evictImmediatelyOnHoldRelease is set, evict on release if all holds are gone.
final boolean isMounted = weakCacheEntry.cacheEntry.isMounted();
- if ((isNewEntry && !isMounted)
- || (areWeakEntriesEphemeral && !weakCacheEntry.isHeld())) {
+ if ((isNewEntry && !isMounted) || areWeakEntriesEphemeral) {
Review Comment:
[P2] Failed partial entries can lose restart metadata
A failed mount consumes the onUnmount hook and deletes the info file. If another hold remains, this guard retains the unmounted entry; the final non-creator hold does not remove it. A later acquire reuses it without rewriting the info file or restoring the hook, so bootstrap can lose the segment after restart.
##########
server/src/main/java/org/apache/druid/segment/loading/StorageLocation.java:
##########
@@ -604,11 +617,14 @@ private Runnable createWeakEntryReleaseRunnable(
weakCacheEntries.computeIfPresent(
weakEntry.cacheEntry.getId(),
(cacheEntryIdentifier, weakCacheEntry) -> {
+ if (weakCacheEntry != weakEntry || weakCacheEntry.isHeld()) {
Review Comment:
[P1] Replacement entry can be mistaken for the in-flight entry
If an abandoned mount entry is reclaimed and a replacement with the same ID is registered, the mount verification and reservation adjustment check only the ID. The old mount can therefore commit against the replacement's reservation and leave its mapper and files live. Verify object identity before committing or adjusting.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: more chill abandoned partial loads and safer deferred eviction of weak held entries (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis merged PR #20073:
URL: https://github.com/apache/druid/pull/20073
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: more chill abandoned partial loads and safer deferred eviction of weak held entries (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #20073:
URL: https://github.com/apache/druid/pull/20073#discussion_r3853030931
##########
server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java:
##########
@@ -950,6 +958,15 @@ private ReservedPartial findOrReservePartial(DataSegment dataSegment, SegmentRan
{
final ReservedPartial existing = findExistingPartialWithHold(dataSegment.getId());
if (existing != null) {
+ if (!existing.metadata().isMounted()) {
+ // rewrite the info file if it is missing
+ try {
+ storeInfoFile(dataSegment);
Review Comment:
[P2] Restored info files lose their cleanup hook
A failed mount consumes its one-shot onUnmount hook while deleting the info file. Reusing that unmounted entry recreates the file here but never registers a new hook, so eviction leaves stale metadata and can cause bootstrap to treat the headerless directory as a complete segment. Re-register the cleanup hook when restoring the file.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: more chill abandoned partial loads and safer deferred eviction of weak held entries (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #20073:
URL: https://github.com/apache/druid/pull/20073#discussion_r3847182158
##########
server/src/test/java/org/apache/druid/segment/loading/SegmentLocalCacheManagerPartialAcquireTest.java:
##########
@@ -962,6 +965,188 @@
);
}
+ @Test
+ void testAbandonedAcquireLeavesAConcurrentAcquiresEntryAlone()
+ throws ExecutionException, InterruptedException, IOException
+ {
+ final StorageLocation location = manager.getLocations().get(0);
+ final SegmentCacheEntryIdentifier id = new SegmentCacheEntryIdentifier(SEGMENT_ID);
+
+ // Two acquires holding the same freshly reserved entry, neither of which resolves its future, so nothing mounts
+ // it. The acquire that created the entry gives up first - a canceled or timed out query, say.
+ final PartialSegmentMetadataCacheEntry entry;
+ final Closer acquires = Closer.create();
+ try {
+ // The first acquire reserves the entry; the second joins it. Both are registered with the closer so a failed
+ // assertion cannot leak a hold into the shared manager fixture; AcquireSegmentAction.close() is idempotent, so
+ // closing the first one early below is safe.
+ final AcquireSegmentAction creator = acquires.register(
+ manager.acquireSegment(partialSegment, AcquireMode.PARTIAL)
+ );
+ acquires.register(manager.acquireSegment(partialSegment, AcquireMode.PARTIAL));
+ entry = Assertions.assertInstanceOf(PartialSegmentMetadataCacheEntry.class, location.getCacheEntry(id));
+ Assertions.assertFalse(entry.isMounted());
+
+ creator.close();
+ Assertions.assertSame(
+ entry,
+ location.getCacheEntry(id),
+ "the entry the second acquire is holding must survive the first giving up"
+ );
+ }
+ finally {
+ acquires.close();
+ }
+ // The second acquire letting go does not remove it either - removal is the creating hold's job - so it is left
+ // registered and unmounted, reclaimable, and reusable by the next acquire.
+ Assertions.assertSame(entry, location.getCacheEntry(id));
+ Assertions.assertFalse(entry.isMounted());
+
+ // A later acquire mounts that same entry and serves the segment from it.
+ try (AcquireSegmentAction action = manager.acquireSegment(partialSegment, AcquireMode.PARTIAL)) {
+ final AcquireSegmentResult result = action.getSegmentFuture().get();
+ try (Segment segment = result.getReferenceProvider().acquireReference().orElseThrow()) {
+ Assertions.assertEquals(SEGMENT_ID, segment.getId());
+ final TimeBoundaryInspector inspector = segment.as(TimeBoundaryInspector.class);
+ Assertions.assertNotNull(inspector);
+ Assertions.assertEquals(TIME, inspector.getMinTime());
+ Assertions.assertEquals(TIME.plusMinutes(3), inspector.getMaxTime());
+ }
+ Assertions.assertSame(entry, location.getCacheEntry(id));
+ Assertions.assertTrue(entry.isMounted());
+ }
+ }
+
+ @Test
+ void testAbandonedAcquireWhoseMountFailsResolvesToAnUnavailableSegmentToo() throws Exception
+ {
+ // Deep storage that can produce a range reader now but not serve a read later: openRangeReader() checks the V10
+ // file at acquire time, and the test deletes it before the load task gets to run.
+ final File vanishingStorage = temporaryFolder.newFolder("vanishing_storage");
+ final File v10File = new File(vanishingStorage, IndexIO.V10_FILE_NAME);
+ Files.copy(new File(DEEP_STORAGE_DIR, IndexIO.V10_FILE_NAME).toPath(), v10File.toPath());
+ final DataSegment vanishingSegment = DataSegment.builder(SEGMENT_ID)
+ .shardSpec(NoneShardSpec.instance())
+ .loadSpec(Map.of("type", "local", "path", vanishingStorage.getAbsolutePath()))
+ .size(0)
+ .build();
+
+ final File gatedCacheRoot = temporaryFolder.newFolder("gated_cache_mount_failure");
+ final SegmentLoaderConfig gatedConfig = SegmentLoaderConfig.builder()
+ .locations(new StorageLocationConfig(gatedCacheRoot, 1024L * 1024L * 1024L, null))
+ .virtualStorage(true)
+ .virtualStoragePartialDownloadsEnabled(true)
+ .virtualStorageUseVirtualThreads(false)
+ .virtualStorageLoadThreads(1)
+ .build();
+ final List<StorageLocation> storageLocations = gatedConfig.toStorageLocations();
+ final StorageLoadingThreadPool gatedPool = StorageLoadingThreadPool.createFromConfig(gatedConfig);
+ final SegmentLocalCacheManager gatedManager = new SegmentLocalCacheManager(
+ storageLocations,
+ gatedConfig,
+ gatedPool,
+ new LeastBytesUsedStorageLocationSelectorStrategy(storageLocations),
+ TestHelper.getTestIndexIO(jsonMapper, ColumnConfig.DEFAULT),
+ jsonMapper
+ );
+
+ final CountDownLatch atGate = new CountDownLatch(1);
+ final CountDownLatch openGate = new CountDownLatch(1);
+ try {
+ // (intentionally unused) local so errorprone's CheckReturnValue is satisfied
+ @SuppressWarnings("unused")
+ ListenableFuture<?> unused = gatedPool.getExecutorService().submit(() -> {
+ atGate.countDown();
+ return openGate.await(30, TimeUnit.SECONDS);
+ });
+ Assertions.assertTrue(atGate.await(30, TimeUnit.SECONDS), "loading thread must reach the gate");
+
+ final ListenableFuture<AcquireSegmentResult> future;
+ try (AcquireSegmentAction action = gatedManager.acquireSegment(vanishingSegment, AcquireMode.PARTIAL)) {
+ future = action.getSegmentFuture();
+ }
+ // The mount will now fail on its header read rather than rolling back cleanly, so the loss surfaces from
+ // mount() instead of from the pin - which is no reason to fail a query that has already given up.
+ Assertions.assertTrue(v10File.delete(), "test needs the deep-storage file gone before the mount runs");
+ openGate.countDown();
+
+ final AcquireSegmentResult result = future.get(30, TimeUnit.SECONDS);
+ Assertions.assertTrue(
+ result.getReferenceProvider().acquireReference().isEmpty(),
+ "an abandoned acquire whose mount failed must resolve to an unavailable segment"
+ );
+ }
+ finally {
+ openGate.countDown();
+ gatedManager.drop(vanishingSegment);
+ gatedManager.shutdown();
+ gatedPool.stop();
+ }
+ }
+
+ @Test
+ void testAbandonedAcquireResolvesToAnUnavailableSegmentRatherThanFailing() throws Exception
+ {
+ // One fixed loading thread, so the test can hold the load task at a gate while it abandons the acquire.
+ final File gatedCacheRoot = temporaryFolder.newFolder("gated_cache");
+ final SegmentLoaderConfig gatedConfig = SegmentLoaderConfig.builder()
+ .locations(new StorageLocationConfig(gatedCacheRoot, 1024L * 1024L * 1024L, null))
+ .virtualStorage(true)
+ .virtualStoragePartialDownloadsEnabled(true)
+ .virtualStorageUseVirtualThreads(false)
+ .virtualStorageLoadThreads(1)
+ .build();
+ final List<StorageLocation> storageLocations = gatedConfig.toStorageLocations();
+ final StorageLoadingThreadPool gatedPool = StorageLoadingThreadPool.createFromConfig(gatedConfig);
+ final SegmentLocalCacheManager gatedManager = new SegmentLocalCacheManager(
+ storageLocations,
+ gatedConfig,
+ gatedPool,
+ new LeastBytesUsedStorageLocationSelectorStrategy(storageLocations),
+ TestHelper.getTestIndexIO(jsonMapper, ColumnConfig.DEFAULT),
+ jsonMapper
+ );
+
+ final CountDownLatch atGate = new CountDownLatch(1);
+ final CountDownLatch openGate = new CountDownLatch(1);
+ try {
+ // (intentionally unused) local so errorprone's CheckReturnValue is satisfied
+ @SuppressWarnings("unused")
+ ListenableFuture<?> unused = gatedPool.getExecutorService().submit(() -> {
+ atGate.countDown();
+ return openGate.await(30, TimeUnit.SECONDS);
+ });
Review Comment:
## CodeQL / Unread local variable
Variable 'ListenableFuture<?> unused' is never read.
[Show more details](https://github.com/apache/druid/security/code-scanning/11910)
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: more chill abandoned partial loads and safer deferred eviction of weak held entries (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #20073:
URL: https://github.com/apache/druid/pull/20073#discussion_r3847458248
##########
server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java:
##########
@@ -950,6 +958,15 @@ private ReservedPartial findOrReservePartial(DataSegment dataSegment, SegmentRan
{
final ReservedPartial existing = findExistingPartialWithHold(dataSegment.getId());
if (existing != null) {
+ if (!existing.metadata().isMounted()) {
+ // rewrite the info file if it is missing
+ try {
+ storeInfoFile(dataSegment);
Review Comment:
Two things:
1) The code that writes info files could use a cleanup. I think it would improve things to run it all through `rewriteInfoFile`, i.e., make `storeInfoFile` call `rewriteInfoFile`, and also make the `if (hold != null)` branch in `acquireSegment` call `rewriteInfoFile`. It would be easier to track through all the places that can write an info file.
2) Do we need to register a hook here to delete the info file on unmount?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: more chill abandoned partial loads and safer deferred eviction of weak held entries (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #20073:
URL: https://github.com/apache/druid/pull/20073#discussion_r3810167815
##########
server/src/test/java/org/apache/druid/segment/loading/SegmentLocalCacheManagerPartialAcquireTest.java:
##########
@@ -915,6 +916,107 @@
);
}
+ @Test
+ void testAcquireMountsALingeringNeverMountedEntryRatherThanRegisteringAnotherOne()
+ throws ExecutionException, InterruptedException, IOException
+ {
+ final StorageLocation location = manager.getLocations().get(0);
+ final SegmentCacheEntryIdentifier id = new SegmentCacheEntryIdentifier(SEGMENT_ID);
+
+ // Two acquires holding the same freshly reserved entry, neither of which resolves its future, so nothing mounts
+ // it. The acquire that created the entry gives up first, so the entry outlives the hold that would have removed
+ // it, and once the second lets go too it is left registered, unmounted and unheld.
+ final AcquireSegmentAction creator = manager.acquireSegment(partialSegment, AcquireMode.PARTIAL);
+ final AcquireSegmentAction other = manager.acquireSegment(partialSegment, AcquireMode.PARTIAL);
+ final PartialSegmentMetadataCacheEntry lingering =
+ Assertions.assertInstanceOf(PartialSegmentMetadataCacheEntry.class, location.getCacheEntry(id));
+ Assertions.assertFalse(lingering.isMounted());
+
+ creator.close();
+ Assertions.assertSame(lingering, location.getCacheEntry(id), "an entry another acquire holds must survive");
+ other.close();
+ Assertions.assertSame(lingering, location.getCacheEntry(id));
+ Assertions.assertFalse(lingering.isMounted());
+
+ // A later acquire mounts that same entry and serves the segment from it, rather than registering a second entry
+ // for this id (two entries would fight over the on-disk state they share).
+ try (AcquireSegmentAction action = manager.acquireSegment(partialSegment, AcquireMode.PARTIAL)) {
+ final AcquireSegmentResult result = action.getSegmentFuture().get();
+ try (Segment segment = result.getReferenceProvider().acquireReference().orElseThrow()) {
+ Assertions.assertEquals(SEGMENT_ID, segment.getId());
+ final TimeBoundaryInspector inspector = segment.as(TimeBoundaryInspector.class);
+ Assertions.assertNotNull(inspector);
+ Assertions.assertEquals(TIME, inspector.getMinTime());
+ Assertions.assertEquals(TIME.plusMinutes(3), inspector.getMaxTime());
+ }
+ Assertions.assertSame(lingering, location.getCacheEntry(id));
+ Assertions.assertTrue(lingering.isMounted());
+ }
+ }
+
+ @Test
+ void testAbandonedAcquireResolvesToAnUnavailableSegmentRatherThanFailing() throws Exception
+ {
+ // One fixed loading thread, so the test can hold the load task at a gate while it abandons the acquire.
+ final File gatedCacheRoot = new File(perTestTempDir, "gated_cache");
+ FileUtils.mkdirp(gatedCacheRoot);
+ final SegmentLoaderConfig gatedConfig = SegmentLoaderConfig.builder()
+ .locations(new StorageLocationConfig(gatedCacheRoot, 1024L * 1024L * 1024L, null))
+ .virtualStorage(true)
+ .virtualStoragePartialDownloadsEnabled(true)
+ .virtualStorageUseVirtualThreads(false)
+ .virtualStorageLoadThreads(1)
+ .build();
+ final List<StorageLocation> storageLocations = gatedConfig.toStorageLocations();
+ final StorageLoadingThreadPool gatedPool = StorageLoadingThreadPool.createFromConfig(gatedConfig);
+ final SegmentLocalCacheManager gatedManager = new SegmentLocalCacheManager(
+ storageLocations,
+ gatedConfig,
+ gatedPool,
+ new LeastBytesUsedStorageLocationSelectorStrategy(storageLocations),
+ TestHelper.getTestIndexIO(jsonMapper, ColumnConfig.DEFAULT),
+ jsonMapper
+ );
+
+ final CountDownLatch atGate = new CountDownLatch(1);
+ final CountDownLatch openGate = new CountDownLatch(1);
+ try {
+ @SuppressWarnings("unused")
+ ListenableFuture<?> unused = gatedPool.getExecutorService().submit(() -> {
+ atGate.countDown();
+ return openGate.await(30, TimeUnit.SECONDS);
+ });
Review Comment:
## CodeQL / Unread local variable
Variable 'ListenableFuture<?> unused' is never read.
[Show more details](https://github.com/apache/druid/security/code-scanning/11881)
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org