FrankChen021 commented on code in PR #20267:
URL: https://github.com/apache/druid/pull/20267#discussion_r3989502815
##########
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:
## Follow-up assessment
I rechecked 13 of 13 changed files at head
`7a6d039a7d0ce63210b702b6d93bf8797ae61b5e`. The proposed bootstrap migration is
not present in this PR, and `evictStaleNonPartialWeakEntry` still removes an
unheld `CompleteSegmentCacheEntry` before the partial reservation/eager
realization begins. If that reload fails, the historical keeps its timeline and
announcement but cannot restore the discarded complete cache entry. This P1
therefore remains open for the current head; please retain the complete entry
until the partial state commits, or remove the timeline/announcement when
conversion fails.
<!-- mergelens:review -->
##########
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);
final DataSegment loaded;
try {
loaded = segmentManager.loadSegment(segment);
}
catch (Exception e) {
- removeSegment(segment, DataSegmentChangeCallback.NOOP, false);
+ if (isReload) {
Review Comment:
## Follow-up assessment
I rechecked 13 of 13 changed files at head
`7a6d039a7d0ce63210b702b6d93bf8797ae61b5e`. The union pinning now fixes the
in-memory hold rollback for an eager-download failure, but `loadPartial` has
already rewritten the info file with the new rule before this restoration path
runs. A restart can therefore retry the failed new rule from disk and, on
another bootstrap failure, drop the last good replica; the current test only
checks the in-memory fingerprint and holds. Please make the info-file update
part of the same commit point as realization, or restore the prior file on
failure. I’m tracking this as a P1 for the current head.
<!-- mergelens:review -->
--
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]
