feat: wire up partial load rules to segment cache#19657
Conversation
capistrant
left a comment
capistrant
left a comment
There was a problem hiding this comment.
nothing I see as outright blocking. left a few comments and nits.
| "Backend for segment[%s] does not support range reads; partial-load rule[fingerprint=%s] cannot be honored, " | ||
| + "falling back to weak full-load at query time", |
There was a problem hiding this comment.
I agree with the decision to fallback here, but how will we help an operator get out of this state? do you think that will come in via documentation of the feature, guiding an operator on how to configure and common pitfalls (like zipped segments resulting in logs like these, and what to change to fix)?
| continue; | ||
| } | ||
| try { | ||
| FileUtils.mkdirp(partialDir); |
There was a problem hiding this comment.
not seeing this cleaned up in the event of failure and rollback. does that need to cleaned up outside of the callable if it fails and rolls back?
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 2 |
| P3 | 0 |
| Total | 2 |
This is an automated review by Codex GPT-5.5
| "Dropping stale static partial entry for segment[%s]: current rule cannot be applied without range reads", | ||
| dataSegment.getId() | ||
| ); | ||
| drop(dataSegment); |
There was a problem hiding this comment.
[P2] Preserve stale static partials while refs are active
The range-reader-null fallback drops a stale static partial entry and immediately deletes stale segment directories without the outstanding-reference precheck used by the normal reconciliation path. If a query still holds a metadata or bundle reference, drop only removes the entry from the static map while actual cleanup is deferred; deleting/reusing the directory before those refs close can break in-flight reads or let deferred cleanup delete files for the next entry. This path should fail/retry when the old partial would defer cleanup.
| // bundles installed by {@link #loadPartial}, this is the only path that removes them from the static map. | ||
| if (entry instanceof PartialSegmentMetadataCacheEntry partial) { | ||
| for (PartialSegmentBundleCacheEntry bundle : partial.snapshotLinkedBundles()) { | ||
| location.release(bundle); |
There was a problem hiding this comment.
[P2] Evict weak linked bundles during partial drops
The new drop cascade calls location.release for each linked bundle, but release is a no-op for weak/on-demand bundles. A rule-loaded partial segment can still have non-selected bundles mounted through the weak path; those bundles hold metadata references for their lifetime, so dropping/reconciling the static metadata can proceed, delete/recreate the same segment directory, and leave old weak bundle entries to later evict containers through the old mapper. Drop/reconcile needs to remove unheld weak linked bundles too, or refuse to proceed while any linked weak bundle remains active.
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 2 |
| P3 | 0 |
| Total | 2 |
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 2 |
| P3 | 0 |
| Total | 2 |
Reviewed 16 of 16 changed files.
Found 2 lifecycle issues in the partial-load-rule reconciliation/drop paths.
This is an automated review by Codex GPT-5.5
| "Dropping stale static partial entry for segment[%s]: current rule cannot be applied without range reads", | ||
| dataSegment.getId() | ||
| ); | ||
| drop(dataSegment); |
There was a problem hiding this comment.
[P2] Range-reader fallback drops live partial entries
This unsupported-range-reader fallback calls drop() and then removes every stale per-segment directory without the cascadeReleaseWouldDeferCleanup() guard used by the main reconciliation path. If a query is still holding the old static partial metadata or bundle when a changed rule falls back because the delegate has no range reader, drop() can defer old cleanup while removeStaleSegmentDirsOnAllLocations() deletes or reuses the same cache directory underneath that query. Please apply the same outstanding-reference precheck here and retry later instead of dropping when cleanup would defer.
| // bundles installed by {@link #loadPartial}, this is the only path that removes them from the static map. | ||
| if (entry instanceof PartialSegmentMetadataCacheEntry partial) { | ||
| for (PartialSegmentBundleCacheEntry bundle : partial.snapshotLinkedBundles()) { | ||
| location.release(bundle); |
There was a problem hiding this comment.
[P2] Weak linked bundles survive partial drop
location.release(bundle) only removes entries from staticCacheEntries, so it is a no-op for weak/on-demand bundles linked to a static partial metadata entry. Those weak bundles still hold metadata references, but the reconciliation precheck subtracts all linked bundles as if drop will release them, after which the new code can nuke or reuse the segment directory while the weak bundles remain mounted. Drop/reconcile should either remove unheld weak linked bundles or treat their presence as a deferred-cleanup blocker.
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).