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: [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]
