FrankChen021 commented on code in PR #20247:
URL: https://github.com/apache/druid/pull/20247#discussion_r3968483130
##########
server/src/main/java/org/apache/druid/server/coordinator/duty/CloneHistoricals.java:
##########
@@ -285,4 +313,66 @@ private boolean shouldLoadSegmentOnTargetServer(
final PartialLoadProfile targetProfile =
targetServer.getProjectedProfile(segment);
return !Objects.equals(fingerprintOf(sourceProfile),
fingerprintOf(targetProfile));
}
+
+ private boolean isSynced(CloningStats stats, CloneSyncCriteria criteria)
+ {
+ return stats.segmentsPendingSync <= criteria.getMaxSegmentsPendingSync()
+ && stats.percentPendingSync() <= criteria.getMaxPercentPendingSync();
Review Comment:
The current head resolves the original contract mismatch: CloneSyncCriteria
now documents “and” and isSynced uses &&. The new test also exercises both
thresholds together. I reviewed all 17 current changed files; no further action
is needed on this thread.
<!-- mergelens:review -->
##########
server/src/main/java/org/apache/druid/server/coordinator/duty/CloneHistoricals.java:
##########
@@ -110,15 +113,19 @@ public DruidCoordinatorRuntimeParams
run(DruidCoordinatorRuntimeParams params)
continue;
}
+ final CloningStats cloningStats = new
CloningStats(sourceServer.getServer().getNumSegments());
+ targetHistoricalStats.put(targetHistoricalName, cloningStats);
+
final Set<DataSegment> sourceProjectedSegments =
sourceServer.getProjectedSegments();
final Set<DataSegment> targetProjectedSegments =
targetServer.getProjectedSegments();
// Load any segment that the clone target is missing, or that it holds
under a different partial-load profile
// than the source. Segment identity alone can't tell those apart: two
replicas of the same segment id may hold
// different parts of it.
for (DataSegment segment : sourceProjectedSegments) {
final PartialLoadProfile sourceProfile =
sourceServer.getProjectedProfile(segment);
- if (shouldLoadSegmentOnTargetServer(segment, sourceProfile,
targetServer, targetProjectedSegments)) {
- loadSegmentOnTargetServer(segment, sourceProfile, targetServer,
params);
+ if (shouldLoadSegmentOnTargetServer(segment, sourceProfile,
targetServer, targetProjectedSegments)
+ && loadSegmentOnTargetServer(segment, sourceProfile, targetServer,
params)) {
Review Comment:
The new condition fixes the pre-existing LOAD case, but the broader issue
remains at this line. A source-served segment with a queued DROP, or a load
that is rejected or rolled back, is not counted because isLoadingSegment is
false; with permissive criteria the status can still be SYNCED while the target
is not serving it. I recorded this remaining [P1] edge case in the current
review after reviewing all 17 changed files.
<!-- 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]
