FrankChen021 commented on code in PR #20267:
URL: https://github.com/apache/druid/pull/20267#discussion_r3968515839
##########
server/src/main/java/org/apache/druid/server/coordinator/loading/StrategicSegmentAssigner.java:
##########
@@ -516,25 +520,68 @@ private int loadPartialReplicas(
return 0;
}
- // The classifier's list is already the complete candidate set when
nothing was canceled.
+ // The classifier's lists are already the complete candidate sets when
nothing was canceled.
+ final List<ServerHolder> inPlaceDestinations;
final List<ServerHolder> freshCandidates;
if (canceledStaleServers.isEmpty()) {
+ inPlaceDestinations = status.getEligibleForInPlaceReload();
freshCandidates = status.getEligibleForFreshLoad();
} else {
+ inPlaceDestinations = new
ArrayList<>(status.getEligibleForInPlaceReload());
freshCandidates = new ArrayList<>(status.getEligibleForFreshLoad());
- freshCandidates.addAll(canceledStaleServers);
+ for (ServerHolder server : canceledStaleServers) {
+ if (server.isServingSegment(segment) &&
PartialSegmentStatusInTier.canReloadInPlace(server)) {
+ inPlaceDestinations.add(server);
+ } else {
+ freshCandidates.add(server);
+ }
+ }
}
- final Iterator<ServerHolder> destinations = Iterators.concat(
- serversToLoadSegment(segment, tier, freshCandidates),
- status.getEligibleForAdditiveReload().iterator()
+ int numLoadsQueued = queuePartialLoads(
Review Comment:
Thanks for the change, but the current head still has the original P1 in the
real partial-cache path. `SegmentLocalCacheManager.loadPartial` can evict a
stale `CompleteSegmentCacheEntry` before reserving the partial entry, or swap
the existing rule/holds and then call `clearRule()` after an eager bundle
failure; neither restores the previous cache state. With
`SegmentLoadDropHandler.java:173` skipping `removeSegment` (and
`SegmentManager.java:355` skipping `cacheManager.drop`), the old
timeline/announcement can remain while the only cached replica has been deleted
or made evictable. Please make that transition rollback-safe (or clean up when
old state is gone) and add a failure-after-eviction/eager-download test;
retrying the same in-place request is not safe if it no longer serves the old
data.
<!-- 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:
[P1] Preserve cache state on failed in-place reload
This treats any load exception for a segment already in SegmentManager's
timeline as safe to leave announced, but that is not true for the partial-load
path. SegmentLocalCacheManager.loadPartial can evict an existing
CompleteSegmentCacheEntry before reserving the partial entry, or replace an
existing partial rule and then clear the new rule when an eager bundle download
fails; neither path restores the previous cache state. Since this branch skips
removeSegment (and SegmentManager.loadSegment likewise skips
cacheManager.drop), the timeline and old announcement can remain while the only
cached replica has been deleted or made eviction-eligible, so queries can see a
non-serving replica and the coordinator keeps retrying in place. Make the
cache/rule transition rollback-safe or only retain the announcement after
verifying the old state is intact, and add a
failure-after-eviction/eager-download test.
--
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]
