FrankChen021 commented on code in PR #20267:
URL: https://github.com/apache/druid/pull/20267#discussion_r3979252284
##########
server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java:
##########
@@ -1076,6 +1072,11 @@ private DataSegment loadPartial(DataSegment dataSegment)
throws SegmentLoadingEx
return dataSegment;
}
+ // Committed to attempting the rule now. If a stale non-partial cache
entry sits at this segment id (a
+ // complete created by a prior acquireSegment while
virtualStoragePartialDownloadsEnabled=false, for example),
+ // evict it before any partial-entry lookup or reservation.
+ evictStaleNonPartialWeakEntry(dataSegment.getId());
Review Comment:
[P1] Preserve a complete cache entry until the partial reload succeeds
`evictStaleNonPartialWeakEntry` removes the existing unheld
`CompleteSegmentCacheEntry` before the new partial entry is reserved or its
eager downloads succeed. If a historical was bootstrapped with a full cache
entry (for example after partial-download configuration changed) and this
reload then fails because the loading pool rejects the work or deep storage
cannot provide a selected bundle, `realizeRuleOrRestorePrior` can only restore
a prior partial rule; it cannot restore the evicted complete entry. The new
code then clears the attempted rule while
`SegmentManager`/`SegmentLoadDropHandler` retain the existing timeline and
announcement as a reload, so a previously queryable local full replica has been
discarded and the server continues to advertise it. Keep the old complete entry
through the attempt or make failure remove the announcement/timeline instead of
treating the replica as safely retained.
##########
server/src/main/java/org/apache/druid/server/coordination/SegmentLoadDropHandler.java:
##########
@@ -161,12 +161,24 @@ public void addSegment(
currentDropLatch.cancelOrAwait();
}
+ // A load request for a segment this server already serves is a reload,
not a new load. The failure cleanup
+ // below exists to discard the half-materialized state a failed *new*
load leaves behind, and running it for a
+ // reload would instead unannounce and drop a replica that is still
serving.
+ final boolean isReload = segmentManager.isSegmentLoaded(segment);
Review Comment:
[P1] Make reload cleanup use one consistent decision
The handler snapshots `isReload` here, but `SegmentManager.loadSegment`
independently snapshots it again at its own call site, with no segment-level
synchronization covering either check and the load. Loads are submitted
independently by `processRequest`, so a concurrent initial load can add the
segment to the timeline between these checks. If the cache load then fails,
`SegmentManager` sees a reload and preserves the cache, while this stale
`false` makes the handler call `removeSegment`, unannouncing and dropping the
live replica. The reverse ordering can also make cleanup inconsistent. Compute
the cleanup decision once under the same lifecycle lock, or return the decision
from `SegmentManager`, so a failed concurrent load cannot recreate the
availability loss this change is meant to avoid.
--
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]
