[I] Partial segment loading: bootstrap-restored cache entries can fail to download after restart (reused BootstrapRangeReader throws) (druid)

["qgambit2 (via GitHub)" <gi...@apache.org>]

qgambit2 opened a new issue, #19680:
URL: https://github.com/apache/druid/issues/19680

   ### Affected version
   
   `master` (feature introduced in #19535, "on-demand partial segment loading"). The feature is gated off by default behind `druid.segmentCache.virtualStorage` + `druid.segmentCache.virtualStoragePartialDownloadsEnabled`.
   
   > Note: this was found by static analysis while reading the #19535 change; I have **not** reproduced it at runtime yet (a repro sketch is included below). Filing it because the code path looks deterministically broken across restarts and is currently untested.
   
   ### Description
   
   With partial downloads enabled, a segment can be cached with only some of its containers on local disk. After a process restart, `PartialSegmentCacheBootstrap.reserveFromDisk` restores such entries from disk — but builds each restored `PartialSegmentMetadataCacheEntry` with `BootstrapRangeReader.INSTANCE`, whose `readRange()` unconditionally throws:
   
   ```java
   // PartialSegmentCacheBootstrap.BootstrapRangeReader
   public InputStream readRange(String filename, long offset, long length) {
     throw DruidException.defensive(
         "BootstrapRangeReader was asked to fetch [%s] @[%d:%d]; bootstrap should only read from local disk", ...);
   }
   ```
   
   A partially-downloaded segment restores with `isFullyDownloaded() == false` (that requires `downloadedFiles.containsAll(metadata.getFiles().keySet())`, while `restoreBundlesFromDisk` admits any bundle whose container files merely exist). When a query later acquires that segment, `SegmentLocalCacheManager.acquireSegment` opens a real working reader via `tryOpenRangeReader`, but `findOrReservePartial` reuses the pre-existing restored entry and **discards** that reader:
   
   ```java
   final ReservedPartial existing = findExistingPartialWithHold(dataSegment.getId());
   if (existing != null) { return existing; }          // the real rangeReader arg is dropped
   return reservePartial(dataSegment, rangeReader);     // only reached when no entry exists yet
   ```
   
   The reader can't be replaced afterward — the entry's and the file mapper's `rangeReader` fields are both `final`, and `doMount` builds the mapper exactly once. So the download then runs through the restored entry's throw-only `BootstrapRangeReader`:
   
   - **`AcquireMode.FULL`:** `ensureAllDownloaded()` → `downloadContainer` → `streamRangeIntoContainer` → `rangeReader.readRange(...)` → throws.
   - **`AcquireMode.PARTIAL`:** at cursor build, `mapFile` → `ensureFileDownloaded` → `streamRangeIntoContainer` → `rangeReader.readRange(...)` → throws.
   
   ### Impact
   
   After any restart, a query that needs not-yet-cached bytes of a partially-downloaded segment fails with a `DruidException` (defensive) instead of fetching the missing bytes from deep storage — i.e., the feature's core lazy-load path is broken across restarts. Only deployments running partial downloads (the off-by-default experimental flags) are affected.
   
   ### Existing mitigation doesn't cover this path
   
   `doMount`'s cleanup (and `testMountFailureRemovesLingeringWeakEntry`) handle the **mount-time** header-refetch case: on mount failure, `removeUnheldWeakEntry` drops the stale entry. But that only fires on mount failure. When the header file is present the mount **succeeds**, the entry persists, and the later container/file download throws with no fallback. `SegmentLocalCacheManagerPartialAcquireTest` exercises `acquireSegment` + download only via **fresh** reservations (real reader), so the restore-then-download path is untested.
   
   ### Suggested fix
   
   When an on-demand acquire reuses a pre-existing (bootstrap-restored) metadata entry, replace its `BootstrapRangeReader` with the real reader from `tryOpenRangeReader` — or evict + re-reserve the entry via `reservePartial` — so restored partial segments can complete downloads. A regression test that restores a partially-downloaded entry and then drives both a FULL and a PARTIAL acquire would lock it in.
   
   ### Suggested reproduction (not yet run)
   
   1. Enable `druid.segmentCache.virtualStorage=true` and `druid.segmentCache.virtualStoragePartialDownloadsEnabled=true` on a historical/MSQ worker (non-ephemeral, the default).
   2. Run a query that lazily downloads only part of a segment, so its on-disk state is partial.
   3. Restart the process (triggering `bootstrap()` → `reserveFromDisk`).
   4. Run a query touching a column/range that wasn't previously downloaded.
   5. Expected: the missing bytes are fetched from deep storage. Observed (per this trace): `DruidException` from `BootstrapRangeReader.readRange`.
   
   *Traced across `PartialSegmentCacheBootstrap`, `SegmentLocalCacheManager`, `PartialSegmentMetadataCacheEntry`, and `PartialSegmentFileMapperV10` by inspection.*
   


-- 
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.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