fix: Fix clone historicals and inter-historical move handling of partial load profiles by capistrant · Pull Request #19843 · apache/druid

capistrant · 2026-07-31T05:06:35Z

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.

This PR now makes a change to SegmentLocalCacheManager#load to identify a move from a partial load to a full load and properly respond to that by releasing all pinned holds and trying to re-write the info file. This is in place of a work-a-round I was doing in the clone code to properly handle a clone processing a partial to full by doing a drop then try again on next coordinator cycle.

Release note

Fix a partial load rule application bug for inter-historical segment balancing and historical cloning

Key changed/added classes in this PR

CloneHistoriclas
SegmentLoadQueueManager

This PR has:

… loads

+    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

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

FrankChen021 · 2026-07-31T12:33:46Z

+        loadableSegment,
+        targetServer,
+        SegmentAction.LOAD,
+        sourceProfile == null ? null : sourceProfile.asRequest()


[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.

FrankChen021 · 2026-07-31T12:33:46Z

+   */
+  public PartialLoadProfile asRequest()
+  {
+    return loadedBytes == null ? this : forRequest(wrappedLoadSpec, fingerprint);


[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

gianm · 2026-07-31T23:35:06Z

+   * 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


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.

yes, this was me trying to keep the change clone related only. latest branch attempts to make load() properly handle this transition in load profile, which simplifies the clone handling

gianm · 2026-07-31T23:45:39Z

+  {
+    if (targetServer.isLoadingSegment(segment)) {
+      if (targetServer.cancelOperation(SegmentAction.LOAD, segment)) {
+        loadSegmentOnTargetServer(segment, null, targetServer, params);


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.

if the new load() handling is acceptable, we should no longer need to consider this. but yes you are right and I have a commit with this handled explicitly in case there are issues with the proposed changes that make this special handling not needed anymore.

FrankChen021

Severity	Findings
P0	0
P1	1
P2	0
P3	0
Total	1

Reviewed 11 of 11 changed files.

This is an automated review by Codex GPT-5.6-Sol

FrankChen021 · 2026-08-01T14:35:12Z

+  {
+    if (targetServer.isLoadingSegment(segment)) {
+      if (targetServer.cancelOperation(SegmentAction.LOAD, segment)) {
+        loadSegmentOnTargetServer(segment, null, targetServer, params);


[P1] Drop a served partial replica after cancelling its reload

A target can simultaneously serve an existing partial replica and have another partial reload queued—for example, profile A is loaded while profile B is waiting when the source transitions to full. Cancellation succeeds here, but the subsequent plain load runs over the existing A replica. As documented above, the ordinary virtual-storage load path does not clear the applied rule or partial info file, while its plain announcement makes the coordinator record a null profile; later clone cycles therefore consider the target full and never repair it. After cancellation, check whether the target already serves a partial replica and drop it first; only queue the immediate full load when no replica is currently served.

…artial to full load of a segment

capistrant · 2026-08-03T15:36:19Z

+   * the segment using the prior wrapper and re-announce the old rule until the coordinator resyncs.
   */
-  private void writePartialInfoFile(DataSegment segment) throws IOException
+  private void rewriteInfoFile(DataSegment segment) throws IOException


not totally sure this rename was necessary. But since it is now called during a partial --> full migration I took out the partial bit. I'm ok to add back if we want

capistrant · 2026-08-03T16:39:38Z

+      rewriteInfoFile(dataSegment);
+    }
+    catch (IOException e) {
+      log.warn(


as of now I've accepted the consequence that on restart the info file is not desirable as it will partial load to the prior state. the coordinator should quickly try to reconcile though

you could flip the order i think if you wanted to only release the rule holds if you updated the info file

ok ya I flipped and now throw a segment load exception on fail. I convinced myself it was better to get a correct in memory state eagerly and live with sub-optimal restart behavior if the info file update failed. but that seems like it is probably short sighted

kfaraz

minor nitpicks

kfaraz · 2026-08-03T16:31:04Z

+        if (!targetProjectedSegments.contains(segment)
+            || !Objects.equals(
+                fingerprintOf(sourceProfile),
+                fingerprintOf(targetServer.getProjectedProfile(segment))
+            )) {


Maybe put this check in a new method shouldLoadSegmentOnTargetServer()

kfaraz · 2026-08-03T16:47:25Z

+   *   <li>{@code loadedBytes} is dropped. A profile read back off a server carries the footprint that server
+   *       realized, which belongs to that server's announcement and not to a request.</li>
+   *   <li>The wrapper's {@link PartialLoadSpec#DELEGATE_FIELD} is replaced with {@code segment}'s load spec. The
+   *       wrapper was built when the source server was asked to load, so it carries whatever deep-storage location


Is this needed? I think in most places, we already assume that the load spec (and the payload in general) of a DataSegment is immutable.
Is that not applicable for the partial-load segments?

I think you are right. got talked into needing to be defensive by an agent review who convinced me I could be handing back a stale request. But I cannot find evidence that it could possibly be stale so it looks to me like useless behavior. will revert

kfaraz · 2026-08-03T16:53:28Z

+   * carrying no load spec, or one whose load spec is already a partial-load wrapper (an outbound request segment
+   * rather than the metadata view, which would otherwise nest one wrapper inside another).
+   */
+  public PartialLoadProfile asRequestFor(DataSegment segment)


Nit: rename the method to indicate that this method modifies the profile for loading on a different server. (I feel the "clone" prefix serves the "move" case too, since there too, we first clone the segment on target server and then delete it from the source server)

Suggested change

public PartialLoadProfile asRequestFor(DataSegment segment)

public PartialLoadProfile asCloneRequestFor(DataSegment segment)

kfaraz · 2026-08-03T16:54:59Z

+    // `segment` is the metadata-resolved segment (see TierSegmentBalancer.getLoadableSegment), which is what
+    // asRequestFor needs to rebase the request onto the segment's current location.


This comment should probably move into the javadoc of getProjectedProfile() method.

clintropolis · 2026-08-03T17:41:44Z

+      rewriteInfoFile(dataSegment);
+    }
+    catch (IOException e) {
+      log.warn(


you could flip the order i think if you wanted to only release the rule holds if you updated the info file

clintropolis · 2026-08-03T17:45:37Z

+              continue;
+            }
+            cacheEntry.setOnUnmount(null);
+            if (isFullLoadRequest


do you need to check this here? if this is true, we already should have called loadPartial instead

good point, after analysis, I think it is at best a waste of time. dropping

capistrant added 2 commits July 30, 2026 23:38

fix bug in historical clones when source and/or target enable partial…

adf1332

… loads

fix MOVE_TO as well

9fcf1d0

capistrant added the Bug label Jul 31, 2026

github-advanced-security AI found potential problems Jul 31, 2026

View reviewed changes

FrankChen021 reviewed Jul 31, 2026

View reviewed changes

capistrant added 2 commits July 31, 2026 09:45

Address p2 from review - avoid stale load profile application

5925fc9

Address P1 from review. if a clone has to full load something it has …

5226009

…partial loaded. drop and let next cycle load it

github-actions Bot added the Area - Segment Format and Ser/De label Jul 31, 2026

Merge branch 'master' into clone-historical-partial-fix

9cf1b86

gianm reviewed Jul 31, 2026

View reviewed changes

FrankChen021 reviewed Aug 1, 2026

View reviewed changes

capistrant added 2 commits August 3, 2026 10:15

Attempt to fixup the workaround for partial to full transition

a5fafe4

Fix the underlying blocker for clones to be able to cleanly go from p…

624cdaa

…artial to full load of a segment

capistrant commented Aug 3, 2026

View reviewed changes

kfaraz reviewed Aug 3, 2026

View reviewed changes

clintropolis reviewed Aug 3, 2026

View reviewed changes

Improvements, cleanups, and fixes based on review comments

a7a7e58

	public PartialLoadProfile asRequestFor(DataSegment segment)
	public PartialLoadProfile asCloneRequestFor(DataSegment segment)

		// `segment` is the metadata-resolved segment (see TierSegmentBalancer.getLoadableSegment), which is what
		// asRequestFor needs to rebase the request onto the segment's current location.

Uh oh!

Conversation

capistrant commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Release note

Key changed/added classes in this PR

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfaraz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

capistrant Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

capistrant commented Jul 31, 2026 •

edited

Loading

capistrant Aug 3, 2026 •

edited

Loading