feat: wire up partial load rules to segment cache#19671
Conversation
changes: * partial load rules/specs are now wired up to the segment cache when virtual storage mode and partial loads are enabled, allowing eager loading of parts of segments which are pinned in the cache until dropped * when under a partial load spec, partial segment cache entries are still added to the cache as weak entries, but with an additional set of 'rule' holds on all required bundles * partial load spec changes operate on a delta, dropping the rule holds for bundles no longer pinned, and loading and adding new rule holds on things which should be pinned
| // fires on doActualUnmount which is triggered by the phaser hitting zero. removeUnheldWeakEntry | ||
| // unlinks the weak entry from cache and terminates the phaser now, firing the hook (and the mapper | ||
| // teardown) on the caller's thread. No-op if a concurrent query still holds the entry. | ||
| location.removeUnheldWeakEntry(id); |
There was a problem hiding this comment.
I'm confused how this work and actually force full cleanup if we don't drive bundle unmounts before we call in to this?
There was a problem hiding this comment.
ah this comment was a bit unclear and your question made me realize that i only intended to eagerly try to evict unheld entries when they were rule held, since I was thinking that stuff that was eagerly partial loads sort of fall in between regular loads with static entries that drop immediately, and pure vsf weak loads, and so my gut feel was that maybe trying to drop if possible was the correct behavior for rule held partials.
However, I realized that this still always results in lazy later eviction because the metadata entries always have a hold on them so they cannot be evicted before a bundle part is evicted, so i've just removed the call completely and so now we just clear the rule.
There was a problem hiding this comment.
got it. ty for explanation and I support decision to drop it
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 2 |
| P2 | 2 |
| P3 | 0 |
| Total | 4 |
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 2 |
| P2 | 2 |
| P3 | 0 |
| Total | 4 |
Reviewed 14 of 14 changed files.
This is an automated review by Codex GPT-5.6-Sol
| continue; | ||
| } | ||
| pending.add(virtualStorageLoadingThreadPool.getExecutorService().submit(() -> { | ||
| metadata.ensureBundleResidentForRule(bundleName); |
There was a problem hiding this comment.
[P1] Download transitive bundle dependencies
ensureBundleResidentForRule downloads only the selected bundle, although mounting a projection may infer __base as a required dependency. The dependency is sparse-mounted and held, but its files are never eagerly downloaded; the otherwise-unused bundlesInMountOrder confirms this missing expansion. Queries can therefore perform on-demand deep-storage reads after the rule was reported fully realized, and fail if deep storage is unavailable.
| // take the existing weak/no-op path below. | ||
| if (config.isVirtualStoragePartialDownloadsEnabled() | ||
| && PartialLoadSpec.detectPartialLoadSpec(dataSegment.getLoadSpec())) { | ||
| loadPartial(dataSegment); |
There was a problem hiding this comment.
[P1] Announce the realized partial footprint
After this new partial path succeeds, SegmentLoadDropHandler still announces the original DataSegment, and unchanged SegmentChangeRequestLoad.forAnnouncement stamps loadedBytes = segment.getSize(). HttpServerInventoryView consequently classifies every successful partial load as a full fallback, overstates server disk usage, and can make coordinator capacity accounting reject otherwise-valid placements. The successful path needs to expose its realized loaded bytes to announcement generation.
| return; | ||
| } | ||
|
|
||
| final ReservedPartial reserved = findOrReservePartial(dataSegment, rangeReader); |
There was a problem hiding this comment.
[P2] Persist rule changes when reusing metadata
findOrReservePartial returns an existing entry without passing through reservePartial, the only partial path that rewrites the info file. A successful fingerprint/selection change is therefore applied in memory while disk retains the prior wrapped load spec; after restart, bootstrap re-applies and announces the old rule until the coordinator corrects it.
| } | ||
| namesToAcquire = new ArrayList<>(); | ||
| for (String name : newSelection) { | ||
| if (!ruleBundleHolds.containsKey(name) && findLinkedBundleByName(name) != null) { |
There was a problem hiding this comment.
[P2] Close the concurrent registration gap
A query can insert a newly mounted bundle into linkedBundles after this snapshot but before ruleSelectedBundleNames is committed. registerBundle observes the old selection and skips its hold, while the eager reconciliation later acquires the already-mounted bundle without invoking registerBundle again. The selected bundle remains permanently absent from ruleBundleHolds and can be SIEVE-evicted despite the applied rule.
3 participants
Description
This PR finishes the partial load rules by wiring the partial load specs up to the segment cache, allowing historicals in virtual storage mode to eagerly load and pin parts of segments to the cache, while allowing the rest of the segment to still be loaded on demand (and evicted). This PR works by storing partial segment cache entries as weak entries, but with an additional 'rule' hold on all parts that are specified by a partial load rule/spec. It takes an imo nicer approach than #19657 which tried to store the rule specified bundles as 'static' cache entries and added a lot of additional complexity for transitions for rule changes. (The load spec changes in this PR should be largely the same as in the other).
changes: