fix: Fix clone historicals and inter-historical move handling of partial load profiles - #19843
fix: Fix clone historicals and inter-historical move handling of partial load profiles#19843capistrant wants to merge 5 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| final long fullSize = Long.parseLong( | ||
| cluster.callApi().runSql( | ||
| "SELECT \"size\" FROM sys.segments WHERE datasource = '" + dataSource + "'" | ||
| ).trim() | ||
| ); |
| final String result = cluster.callApi().runSql( | ||
| "SELECT curr_size FROM sys.servers WHERE server_type = 'historical' AND server = '" + host + "'" | ||
| ).trim(); | ||
| return result.isEmpty() ? 0L : Long.parseLong(result); |
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 1 |
| P3 | 0 |
| Total | 2 |
Reviewed 8 of 8 changed files.
This is an automated review by Codex GPT-5.6-Sol
| loadableSegment, | ||
| targetServer, | ||
| SegmentAction.LOAD, | ||
| sourceProfile == null ? null : sourceProfile.asRequest() |
There was a problem hiding this comment.
[P1] Clear partial state before cloning a full load
When the source profile is null and the target already has a partial profile, this queues a plain LOAD. On a virtual-storage Historical, however, the plain SegmentLocalCacheManager.load path does not clear PartialSegmentMetadataCacheEntry's rule or rewrite its partial info file; SegmentManager also leaves the existing segment in place. The announcer then reports a plain profile, so the coordinator believes the clone is full while the old rule holds remain, and a restart re-applies and re-announces the stale partial profile. Explicitly clear the persisted partial rule for this conversion or perform a drop/reload. The added unit test only verifies the queued request, not this Historical-side transition.
| */ | ||
| public PartialLoadProfile asRequest() | ||
| { | ||
| return loadedBytes == null ? this : forRequest(wrappedLoadSpec, fingerprint); |
There was a problem hiding this comment.
[P2] Rebase forwarded profiles onto the current load spec
wrappedLoadSpec contains the source announcement's original deep-storage delegate, and asRequest carries it forward verbatim. Both new callers start with a DataSegment fetched from the current metadata snapshot, but SegmentHolder subsequently replaces that segment's loadSpec with this stale wrapper. If a segment payload was corrected or migrated since the source loaded, clone and move requests therefore use the old location; after that object is removed, cloning fails and balancing or decommissioning moves cannot complete. Preserve the selection and fingerprint while rebuilding the wrapper with the latest metadata loadSpec as its delegate.
…partial loaded. drop and let next cycle load it
| * Returns the clone target to a full load of {@code segment}, for when the source has stopped holding it partially. | ||
| * <p> | ||
| * A plain load request on top of the existing replica does not achieve this. A historical that receives an unwrapped | ||
| * load request for a segment it already holds under a partial-load rule keeps that rule applied: its holds go on |
There was a problem hiding this comment.
A historical that receives an unwrapped load request for a segment it already holds under a partial-load rule keeps that rule applied
Is this a good thing, or are you working around a Historical bug here? To me it seems to make sense to have the Historical able to handle a new load request in this case, rather than requiring a drop + load.
| { | ||
| if (targetServer.isLoadingSegment(segment)) { | ||
| if (targetServer.cancelOperation(SegmentAction.LOAD, segment)) { | ||
| loadSegmentOnTargetServer(segment, null, targetServer, params); |
There was a problem hiding this comment.
Isn't it possible that a LOAD was in-flight to a target server that already had the segment loaded, so the cancelOperation succeeding doesn't necessarily mean the target doesn't have the segment? I think this could happen from the call to loadSegmentOnTargetServer on line 131, if the projected profile fingerprint had changed.
4 participants
Description
Segment moves and Historical cloning did not have proper handling for partial load profiles. When a segment should have been being loaded partially, it was not.
Release note
Fix a partial load rule application bug for inter-historical segment balancing and historical cloning
Key changed/added classes in this PR
This PR has: