feat: partial load rules assignment prefer in-place reload over fresh load - #20267
feat: partial load rules assignment prefer in-place reload over fresh load#20267clintropolis wants to merge 1 commit into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| * {@link #getEligibleForFreshLoad()}. See {@link StrategicSegmentAssigner#updateReplicasInTierPartial}. | ||
| */ | ||
| public List<ServerHolder> getEligibleForAdditiveReload() | ||
| public List<ServerHolder> getEligibleForInPlaceReload() |
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 0 |
| P3 | 0 |
| Total | 1 |
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 0 |
| P3 | 0 |
| Total | 1 |
Reviewed 5 of 5 changed files.
This is an automated review by Codex GPT-5.6-Luna(max)
| final Iterator<ServerHolder> destinations = Iterators.concat( | ||
| serversToLoadSegment(segment, tier, freshCandidates), | ||
| status.getEligibleForAdditiveReload().iterator() | ||
| int numLoadsQueued = queuePartialLoads( |
There was a problem hiding this comment.
[P1] Preserve the serving replica when an in-place reload fails
This now queues an in-place reload before trying any empty server, but a failed loadPartial is handled by SegmentLoadDropHandler.addSegment as a failed segment load: it unannounces and drops the existing segment. Failures are possible here (for example, the old cache entry is held or the new partial bundles cannot be reserved), and canReloadInPlace intentionally does not check disk capacity. With one stale replica and a viable empty server, this can remove the only serving copy even though the previous fresh-first path would have kept it serving while the replacement loaded. Preserve or roll back the old replica, or fall back to a fresh destination when the in-place load cannot proceed, and add a failure-with-fresh-candidate test.
3 participants
Description
Updates
PartialSegmentStatusInTierandStrategicSegmentAssignerto prefer in-place reloads of partial segments rather than load-then-drop to another server.The reason it was like this is because partial load rules were introduced before historicals could actually honor them (partial loading was still work in progress), and my design at the time meant that they would only do an additive load on rule change. However the functionality that got actually merged (#19671) ended up having the rules using internal holds on the weak references that make up a partially loaded segment in cache, so a rule change is not just additive (we load any missing bundles, and release the holds on any bundles no longer pinned by rules, allowing them to be evicted).
This means that there is no downside to doing an in-place reload, only positives since it only has to download the difference rather than the whole set, so this PR flips the priority to do fresh load as the fallback behavior.