gianm commented on code in PR #20247: URL: https://github.com/apache/druid/pull/20247#discussion_r3961018676
########## docs/operations/metrics.md: ########## @@ -468,6 +471,8 @@ These metrics are emitted by the Druid Coordinator in every run of the correspon |`segment/underReplicated/count`|Number of segments, including replicas, left to load until all used segments are available for queries.|`tier`, `dataSource`|0| |`segment/availableDeepStorageOnly/count`|Number of unique segments that are only available for querying directly from deep storage.|`dataSource`|Varies| |`tier/historical/count`|Number of available historical nodes in each tier. The `tierAlias` dimension is emitted only when the tier belongs to an alias configured via [`historicalTierAliases`](../configuration/index.md#dynamic-configuration), and can be used to aggregate metrics across the tiers in an alias.|`tier`, `tierAlias`|Varies| +|`tier/historical/clone/count`|Number of historical nodes in a tier which are a clone of another historical in the same or different tier. The `tierAlias` dimension is emitted only when the tier belongs to an alias configured via [`historicalTierAliases`](../configuration/index.md#dynamic-configuration), and can be used to aggregate metrics across the tiers in an alias.|`tier`, `tierAlias`|Varies| +|`tier/historical/clone/synced`|Number of historical clones in a tier which are now synced with their source server. |`server`, `tier`|Varies| Review Comment: The wording is vague: is "now synced" meant to mean "newly synced" (i.e. this is a delta since last emit) or is it "currently synced" (i.e. this is a gauge)? Probably a gauge is more useful. ########## server/src/main/java/org/apache/druid/server/coordinator/CloneSyncCriteria.java: ########## @@ -0,0 +1,75 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.server.coordinator; + +import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.druid.common.config.Configs; +import org.apache.druid.error.InvalidInput; + +import javax.annotation.Nullable; + +/** + * Criteria definining when a clone historical should be considered as + * {@link ServerCloneStatus.State#SYNCED} to its source server. The criteria is + * a function of the number or percentage of segments "pending sync", i.e. Review Comment: Should be "and" not "or" (both number and percentage must be satisfied to be considered synced). ########## server/src/main/java/org/apache/druid/server/coordinator/duty/CloneHistoricals.java: ########## @@ -149,6 +164,7 @@ private void loadSegmentOnTargetServer( { final RowKey.Builder rowKey = RowKey .with(Dimension.SERVER, targetServer.getServer().getName()) + .with(Dimension.TIER, targetServer.getServer().getTier()) Review Comment: Maybe this is out of scope for this PR, since it seems like there are other pre-existing metrics that are also missing it, but I think this should attach `tierAlias`. We should attach `tierAlias` everywhere we have `tier`. ########## server/src/main/java/org/apache/druid/server/coordinator/duty/CloneHistoricals.java: ########## @@ -239,10 +265,12 @@ private Map<String, ServerCloneStatus> createCurrentStatusMap( if (targetServer == null) { newStatus = ServerCloneStatus.unknown(sourceServerName, targetServerName); } else { - ServerCloneStatus.State state; + final CloningStats stats = targetHistoricalStats.getOrDefault(targetServerName, new CloningStats(0)); if (!historicalMap.containsKey(sourceServerName)) { state = ServerCloneStatus.State.SOURCE_SERVER_MISSING; Review Comment: Should we go with `ServerCloneStatus.unknown` in this case (as we do when the target is missing)? ########## 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: IMO the `&&` (as implemented here) is the nicer behavior. I would expect either condition being unmet to lead to the server being treated as "not synced". So, rather than update this logic here, I'd say update the Javadoc at `CloneSyncCriteria`. ########## server/src/main/java/org/apache/druid/server/coordinator/CloneSyncCriteria.java: ########## @@ -0,0 +1,75 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.server.coordinator; + +import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.druid.common.config.Configs; +import org.apache.druid.error.InvalidInput; + +import javax.annotation.Nullable; + +/** + * Criteria definining when a clone historical should be considered as + * {@link ServerCloneStatus.State#SYNCED} to its source server. The criteria is + * a function of the number or percentage of segments "pending sync", i.e. + * segments already loaded on the source server but still loading on the target + * server. Segments which are yet to be loaded on the source server itself do not + * affect the sync status. + */ +public record CloneSyncCriteria( Review Comment: These should be in `docs/configuration/index.md` under "Dynamic configuration". ########## 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: Seems reasonable to include these in equality, and add an `EqualsVerifier` test. ########## 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: This sounds legit to me. Even if we haven't issued a load this run, we should still check something like `targetServer.isLoadingSegment(segment) && sourceServer.isServingSegment(segment)` and count those as missing. -- 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]
