gianm commented on code in PR #19843:
URL: https://github.com/apache/druid/pull/19843#discussion_r3693967624
##########
server/src/main/java/org/apache/druid/server/coordinator/duty/CloneHistoricals.java:
##########
@@ -150,6 +182,41 @@ private void loadSegmentOnTargetServer(
}
}
+ /**
+ * 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
+ * pinning the parts the rule selected, and the segment's info file goes on
describing a partial load, which the
+ * historical reapplies and re-announces on its next restart. Dropping the
replica does release the rule and retire
+ * the info file, so the next coordinator run sees a clone that is missing
the segment and queues the ordinary full
+ * load.
+ * <p>
+ * A partial load that is still queued is cancelled and replaced by the full
load within this run, since nothing has
+ * been applied on the historical yet. If the request has already gone out,
the cancel fails and that load runs to
+ * completion; the drop path then converts the replica on a later run.
+ */
+ private void convertCloneReplicaToFullLoad(
+ DataSegment segment,
+ ServerHolder targetServer,
+ DruidCoordinatorRuntimeParams params
+ )
+ {
+ if (targetServer.isLoadingSegment(segment)) {
+ if (targetServer.cancelOperation(SegmentAction.LOAD, segment)) {
+ loadSegmentOnTargetServer(segment, null, targetServer, params);
Review 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.
##########
server/src/main/java/org/apache/druid/server/coordinator/duty/CloneHistoricals.java:
##########
@@ -150,6 +182,41 @@ private void loadSegmentOnTargetServer(
}
}
+ /**
+ * 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
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
