FrankChen021 commented on code in PR #20247:
URL: https://github.com/apache/druid/pull/20247#discussion_r3933953656
##########
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:
[P1] Allow either sync threshold to qualify
The two configured limits are intersected here, so `maxSegmentsPendingSync`
cannot independently make a clone `SYNCED`. The new test configures
`(maxSegmentsPendingSync=1, maxPercentPendingSync=10.0)` with nine source
segments and one pending segment: the absolute limit is met, but the calculated
percentage is 11.11%, leaving the status `IN_PROGRESS` and contradicting the
test's expected `SYNCED` state. Combine the thresholds according to the
documented number-or-percentage semantics (or change the public contract and
test).
##########
server/src/main/java/org/apache/druid/server/coordinator/CoordinatorDynamicConfig.java:
##########
@@ -77,6 +77,7 @@ public class CoordinatorDynamicConfig
private final Set<String> turboLoadingNodes;
private final Map<String, String> cloneServers;
+ private final CloneSyncCriteria cloneSyncCriteria;
Review Comment:
[P1] Include clone criteria in config equality
The new `cloneSyncCriteria` field is not included in
`CoordinatorDynamicConfig.equals()` or `hashCode()`. The existing
`CoordinatorDynamicConfigTest` uses `EqualsVerifier` without ignoring this
field, so the test will reject the class; in production,
`BaseDynamicConfigSyncer` also treats a threshold-only update as unchanged and
does not invalidate its `inSyncBrokers` snapshot. Add value-based equality for
the criteria and include it in both config methods.
##########
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();
+ }
+
+ /**
+ * Adds metrics to the run stats if the state has changed to SYNCED in this
run.
+ */
+ private void collectMetricsIfStateChanged(
+ Map<String, ServerCloneStatus> targetServerToStatus,
+ CoordinatorRunStats stats
+ )
+ {
+ targetServerToStatus.forEach((targetServerName, newStatus) -> {
+ final ServerCloneStatus oldStatus =
cloneStatusManager.getStatusForServer(targetServerName);
+
+ if (newStatus.state() == ServerCloneStatus.State.SYNCED
+ && oldStatus != null && oldStatus.state() !=
ServerCloneStatus.State.SYNCED) {
Review Comment:
[P2] Emit the initial SYNCED transition metric
A coordinator starts with no entry in `CloneStatusManager`, so a clone that
already satisfies the criteria on its first run has `oldStatus == null`. This
condition then suppresses `Stats.Segments.PENDING_SYNC` even though the new
state is `SYNCED`; the added simulation test expects that metric on its initial
run. Treat the absent status as a transition (or otherwise emit the metric for
the initial qualifying run).
--
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]
