FrankChen021 commented on code in PR #19884:
URL: https://github.com/apache/druid/pull/19884#discussion_r3728798222
##########
server/src/main/java/org/apache/druid/server/coordinator/loading/StrategicSegmentAssigner.java:
##########
@@ -645,20 +653,74 @@ private int updateReplicasInTier(
}
// Cancel loads and queue drops if the projected count exceeds the
requirement
+ int dropsQueuedOnTier = 0;
if (projectedReplicas > requiredReplicas) {
int replicaSurplus = projectedReplicas - requiredReplicas;
int canceledLoads =
cancelOperations(SegmentAction.LOAD, replicaSurplus, segment,
segmentStatus);
int numReplicasToDrop = Math.min(replicaSurplus - canceledLoads,
maxReplicasToDrop);
if (numReplicasToDrop > 0) {
- int dropsQueuedOnTier = dropReplicas(numReplicasToDrop, segment, tier,
segmentStatus);
+ dropsQueuedOnTier = dropReplicas(numReplicasToDrop, segment, tier,
segmentStatus);
incrementStat(Stats.Segments.DROPPED, segment, tier,
dropsQueuedOnTier);
- return dropsQueuedOnTier;
}
}
- return 0;
+ // Release partial-load rules that no longer apply. Done last so the
load/drop decisions above claim their
+ // servers first: a replica that just picked up an action is no longer
`isServingSegment`, so it is skipped here
+ // and reverted on a later run if it is still around.
+ if (replicasToRevert > 0) {
+ final int reverted = revertPartialProfileReplicas(segment, tier);
+ if (reverted > 0) {
+ incrementStat(Stats.Segments.PARTIAL_RULE_REVERTED, segment, tier,
reverted);
+ }
+ }
+
+ return dropsQueuedOnTier;
+ }
+
+ /**
+ * Queues an in-place reload on every server in {@code tier} that serves
{@code segment} under a
+ * {@link PartialLoadProfile}. The request carries the plain unwrapped
{@code segment}, which is what tells the
+ * historical to release its rule holds rather than apply or swap one.
+ * <p>
+ * The replica count is deliberately left alone: these servers <em>are</em>
serving, so they still satisfy the
+ * rule's replication requirement and must not be double-counted as a
deficit. This only refreshes what they hold.
+ * <p>
+ * Servers are skipped when:
+ * <ul>
+ * <li>they have any queued action, via {@link
ServerHolder#isServingSegment}, which covers both the load/drop
+ * decisions made earlier in this run and operations left over from a
previous one;</li>
+ * <li>their load queue is already at the configured {@code
maxSegmentsInNodeLoadingQueue} budget for this run.</li>
+ * <li>they are decommissioning, since their replicas are on the way out
and reloading them is wasted work.</li>
+ * </ul>
+ * These are the same two eligibility conditions {@code
PartialSegmentStatusInTier.canReloadAdditively} applies to
+ * the partial-load reconciler's in-place reload. {@link
ServerHolder#canLoadSegment} is not usable here because it
+ * requires the server to <em>not</em> already have the segment, which is
precisely the case being handled.
+ */
+ private int revertPartialProfileReplicas(DataSegment segment, String tier)
+ {
+ int numReverted = 0;
+ for (ServerHolder server : cluster.getManagedHistoricalsByTier(tier)) {
+ if (revertPartialProfileReplica(segment, server)) {
+ ++numReverted;
+ }
+ }
+ return numReverted;
+ }
+
+ /**
+ * Queues the in-place reload described by {@link
#revertPartialProfileReplicas} on a single server, if that server
+ * is holding {@code segment} under a partial-load rule and has room in its
load queue. Returns whether a reload was
+ * queued.
+ */
+ private boolean revertPartialProfileReplica(DataSegment segment,
ServerHolder server)
+ {
+ return server.isServingSegment(segment)
+ && !server.isDecommissioning()
+ && !server.isLoadQueueFull()
+ && server.getServer().getPartialLoadProfile(segment.getId()) != null
+ && loadQueueManager.loadSegment(segment, server,
SegmentAction.LOAD, null);
Review Comment:
[P2] Keep in-place reverts out of replica surplus accounting
If this in-place reload remains queued into the next coordinator run,
`SegmentReplicaCountMap` counts both the already-announced partial replica and
this `LOAD`. For a one-replica full-load rule, `projectedReplicas` therefore
becomes 2 and `updateReplicasInTier` treats the reload as surplus, canceling it
through `cancelOperations(LOAD)`. `HttpLoadQueuePeon` permits cancellation
until dispatch; under queue backlog the coordinator can repeatedly cancel and
requeue the revert, or with a finite queue budget cancel it and wait another
run, preventing the partial-to-full transition from completing. Represent this
as a non-replica refresh or exclude same-server reloads from replica counting
and surplus cancellation, and add a two-run test with a pending revert.
--
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]
