gianm commented on code in PR #19374: URL: https://github.com/apache/druid/pull/19374#discussion_r3142963573
########## server/src/main/java/org/apache/druid/server/coordinator/rules/WildcardProjectionPartialLoadMatcher.java: ########## @@ -0,0 +1,132 @@ +/* + * 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.rules; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.commons.io.FilenameUtils; +import org.apache.druid.error.InvalidInput; +import org.apache.druid.timeline.DataSegment; + +import javax.annotation.Nullable; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Objects; +import java.util.TreeSet; + +/** + * Selects projections whose names match any of the configured glob patterns, minus any names matching an entry in + * {@code excludePatterns}. Matching uses {@link FilenameUtils#wildcardMatch(String, String)}; supported glob + * metacharacters are {@code *} (any sequence of characters) and {@code ?} (single character), all other characters + * literal. A literal projection name is also a valid glob (with no wildcards it matches exactly itself), so the same + * field covers both "exclude this specific name" and "exclude anything matching this pattern." + * <p> + * For example, a long-retention rule {@code patterns=["user_*"], excludePatterns=["user_daily"]} keeps every + * {@code user_*} projection except {@code user_daily} (which is expected to live on a shorter-retention rule). A + * broad rule {@code patterns=["*"], excludePatterns=["user_*"]} loads every projection except those handled by a + * more specific {@code user_*} rule elsewhere in the cascade. + */ +public class WildcardProjectionPartialLoadMatcher extends ProjectionPartialLoadMatcher +{ + public static final String TYPE = "globProjection"; + + private final List<String> patterns; + private final List<String> excludePatterns; + + @JsonCreator + public WildcardProjectionPartialLoadMatcher( + @JsonProperty("patterns") List<String> patterns, + @JsonProperty("excludePatterns") @Nullable List<String> excludePatterns + ) + { + if (patterns == null || patterns.isEmpty()) { + throw InvalidInput.exception("patterns must not be null or empty for globProjection matcher"); + } + this.patterns = List.copyOf(patterns); + this.excludePatterns = excludePatterns == null ? List.of() : List.copyOf(excludePatterns); + } + + @JsonProperty + public List<String> getPatterns() + { + return patterns; + } + + @JsonProperty Review Comment: `@JsonInclude(NON_EMPTY)` would be nice. ########## server/src/main/java/org/apache/druid/server/coordinator/rules/ProjectionPartialLoadMatcher.java: ########## @@ -0,0 +1,84 @@ +/* + * 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.rules; + +import com.google.common.hash.Hasher; +import com.google.common.hash.Hashing; +import com.google.common.io.BaseEncoding; +import org.apache.druid.timeline.DataSegment; + +import javax.annotation.Nullable; +import java.nio.charset.StandardCharsets; +import java.util.List; +import java.util.Locale; +import java.util.Map; + +/** + * Base for {@link PartialLoadMatcher} implementations that decide which of a segment's V10 projections to load. + * Subclasses supply the resolution policy via {@link #resolveProjectionNames(DataSegment)}; this base handles + * fingerprint computation and wraps the result into the {@code partialProjection} load-spec wire form consumed + * by the historical-side {@code PartialProjectionLoadSpec} (which does not exist yet, supplied in future work). Review Comment: IMO best not to mention things that don't exist yet. Instead, the next PR should update this text. It will need to update it anyway to remove the "supplied in future work" comment. ########## server/src/main/java/org/apache/druid/server/coordinator/rules/CannotMatchBehavior.java: ########## @@ -0,0 +1,60 @@ +/* + * 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.rules; + +import com.fasterxml.jackson.annotation.JsonCreator; + +import javax.annotation.Nullable; + +/** + * Controls what happens when a {@link PartialLoadRule}'s {@link PartialLoadMatcher} does not apply to a given segment, + * for example, a {@link ProjectionPartialLoadMatcher} when faced with a segment that doesn't have projections. + * <p> + * Unknown values deserialize to {@code null} so that an older coordinator encountering a rule authored on a newer + * version that introduces a new behavior falls back to the constructor's default ({@link #FULL_LOAD}) rather than + * failing to parse the rule. + */ +public enum CannotMatchBehavior Review Comment: IMO it'd be nicer to define the enums like `FALL_THROUGH("fallThrough")` and have the `"fallThrough"` be how they serialize in JSON. SImilar to `JoinAlgorithm` and `CloneQueryMode`. Most Druid stuff uses `thatStyle` rather than `THIS_ONE`. ########## server/src/main/java/org/apache/druid/server/coordinator/rules/PartialLoadMatcher.java: ########## @@ -0,0 +1,60 @@ +/* + * 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.rules; + +import com.fasterxml.jackson.annotation.JsonSubTypes; +import com.fasterxml.jackson.annotation.JsonTypeInfo; +import org.apache.druid.segment.loading.LoadSpec; +import org.apache.druid.server.coordination.SegmentChangeRequestLoad; +import org.apache.druid.timeline.DataSegment; + +import javax.annotation.Nullable; +import java.util.Map; + +/** + * Decides whether a {@link PartialLoadRule} should partially load a given segment and, when it should, produces the + * wire-form load-spec wrapper plus a fingerprint identifying that request. Implementations encapsulate both the + * configuration that drives the decision and the wire format of their corresponding {@link LoadSpec} wrapper, so the + * rule layer stays scheme-agnostic. + */ +@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type") Review Comment: If we add a new one of these then the `PartialLoadRule` will fail to deserialize. Just like `CannotMatchBehavior`, maybe this should be lenient too? The way to make things lenient is to add a dummy implementation as a `defaultImpl`, such as `QueryCounterSnapshot` for example. Servers will then at least know that they're dealing with an unrecognized `PartialLoadMatcher`. ########## server/src/main/java/org/apache/druid/server/coordinator/rules/ProjectionPartialLoadMatcher.java: ########## @@ -0,0 +1,84 @@ +/* + * 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.rules; + +import com.google.common.hash.Hasher; +import com.google.common.hash.Hashing; +import com.google.common.io.BaseEncoding; +import org.apache.druid.timeline.DataSegment; + +import javax.annotation.Nullable; +import java.nio.charset.StandardCharsets; +import java.util.List; +import java.util.Locale; +import java.util.Map; + +/** + * Base for {@link PartialLoadMatcher} implementations that decide which of a segment's V10 projections to load. + * Subclasses supply the resolution policy via {@link #resolveProjectionNames(DataSegment)}; this base handles + * fingerprint computation and wraps the result into the {@code partialProjection} load-spec wire form consumed + * by the historical-side {@code PartialProjectionLoadSpec} (which does not exist yet, supplied in future work). + * <p> + * The fingerprint is a hash of what projections are partially loaded on a segment by this rule; the data node will + * include this value in the segment announcement so that it can be used as a lightweight value to compare against + * to handle things like rule change so that we can ensure that the 'right' partial load is in place from run to run. + */ +public abstract class ProjectionPartialLoadMatcher implements PartialLoadMatcher +{ + static final String WIRE_TYPE = "partialProjection"; Review Comment: `LOAD_SPEC_TYPE`? Sounds clearer. ########## server/src/main/java/org/apache/druid/server/coordinator/rules/ProjectionPartialLoadMatcher.java: ########## @@ -0,0 +1,84 @@ +/* + * 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.rules; + +import com.google.common.hash.Hasher; +import com.google.common.hash.Hashing; +import com.google.common.io.BaseEncoding; +import org.apache.druid.timeline.DataSegment; + +import javax.annotation.Nullable; +import java.nio.charset.StandardCharsets; +import java.util.List; +import java.util.Locale; +import java.util.Map; + +/** + * Base for {@link PartialLoadMatcher} implementations that decide which of a segment's V10 projections to load. + * Subclasses supply the resolution policy via {@link #resolveProjectionNames(DataSegment)}; this base handles + * fingerprint computation and wraps the result into the {@code partialProjection} load-spec wire form consumed + * by the historical-side {@code PartialProjectionLoadSpec} (which does not exist yet, supplied in future work). + * <p> + * The fingerprint is a hash of what projections are partially loaded on a segment by this rule; the data node will + * include this value in the segment announcement so that it can be used as a lightweight value to compare against + * to handle things like rule change so that we can ensure that the 'right' partial load is in place from run to run. + */ +public abstract class ProjectionPartialLoadMatcher implements PartialLoadMatcher +{ + static final String WIRE_TYPE = "partialProjection"; + static final String FINGERPRINT_VERSION = "v1"; + + /** + * Returns the sorted, deduped list of projection names from {@link DataSegment#getProjections()} that this matcher + * selects. Returns an empty list when nothing matches (the segment exposes no projections, or no configured pattern + * intersects what the segment has). + */ + protected abstract List<String> resolveProjectionNames(DataSegment segment); + + @Override + @Nullable + public MatchResult match(DataSegment segment, Map<String, Object> baseLoadSpec) + { + final List<String> resolved = resolveProjectionNames(segment); + if (resolved.isEmpty()) { + return null; + } + final String fingerprint = computeFingerprint(resolved); + final Map<String, Object> wrapped = Map.of( + "type", WIRE_TYPE, + "delegate", baseLoadSpec, + "projections", resolved, + "ruleFingerprint", fingerprint + ); + return new MatchResult(wrapped, fingerprint); + } + + static String computeFingerprint(List<String> sortedDedupedNames) + { + final Hasher hasher = Hashing.sha256().newHasher(); + for (String name : sortedDedupedNames) { + hasher.putString(name, StandardCharsets.UTF_8); Review Comment: The javadoc for `putString` says that `putUnencodedChars` is preferred. ########## server/src/main/java/org/apache/druid/server/coordinator/rules/ProjectionPartialLoadMatcher.java: ########## @@ -0,0 +1,84 @@ +/* + * 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.rules; + +import com.google.common.hash.Hasher; +import com.google.common.hash.Hashing; +import com.google.common.io.BaseEncoding; +import org.apache.druid.timeline.DataSegment; + +import javax.annotation.Nullable; +import java.nio.charset.StandardCharsets; +import java.util.List; +import java.util.Locale; +import java.util.Map; + +/** + * Base for {@link PartialLoadMatcher} implementations that decide which of a segment's V10 projections to load. + * Subclasses supply the resolution policy via {@link #resolveProjectionNames(DataSegment)}; this base handles + * fingerprint computation and wraps the result into the {@code partialProjection} load-spec wire form consumed + * by the historical-side {@code PartialProjectionLoadSpec} (which does not exist yet, supplied in future work). + * <p> + * The fingerprint is a hash of what projections are partially loaded on a segment by this rule; the data node will + * include this value in the segment announcement so that it can be used as a lightweight value to compare against + * to handle things like rule change so that we can ensure that the 'right' partial load is in place from run to run. + */ +public abstract class ProjectionPartialLoadMatcher implements PartialLoadMatcher +{ + static final String WIRE_TYPE = "partialProjection"; + static final String FINGERPRINT_VERSION = "v1"; + + /** + * Returns the sorted, deduped list of projection names from {@link DataSegment#getProjections()} that this matcher + * selects. Returns an empty list when nothing matches (the segment exposes no projections, or no configured pattern + * intersects what the segment has). + */ + protected abstract List<String> resolveProjectionNames(DataSegment segment); + + @Override + @Nullable + public MatchResult match(DataSegment segment, Map<String, Object> baseLoadSpec) + { + final List<String> resolved = resolveProjectionNames(segment); + if (resolved.isEmpty()) { + return null; + } + final String fingerprint = computeFingerprint(resolved); + final Map<String, Object> wrapped = Map.of( + "type", WIRE_TYPE, + "delegate", baseLoadSpec, + "projections", resolved, + "ruleFingerprint", fingerprint Review Comment: It isn't really a rule fingerprint, it's more of a projection names fingerprint. Maybe just call it `fingerprint` since `projectionNamesFingerprint` is a mouthful. ########## server/src/main/java/org/apache/druid/server/coordinator/rules/WildcardProjectionPartialLoadMatcher.java: ########## @@ -0,0 +1,132 @@ +/* + * 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.rules; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.commons.io.FilenameUtils; +import org.apache.druid.error.InvalidInput; +import org.apache.druid.timeline.DataSegment; + +import javax.annotation.Nullable; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Objects; +import java.util.TreeSet; + +/** + * Selects projections whose names match any of the configured glob patterns, minus any names matching an entry in + * {@code excludePatterns}. Matching uses {@link FilenameUtils#wildcardMatch(String, String)}; supported glob + * metacharacters are {@code *} (any sequence of characters) and {@code ?} (single character), all other characters + * literal. A literal projection name is also a valid glob (with no wildcards it matches exactly itself), so the same + * field covers both "exclude this specific name" and "exclude anything matching this pattern." + * <p> + * For example, a long-retention rule {@code patterns=["user_*"], excludePatterns=["user_daily"]} keeps every + * {@code user_*} projection except {@code user_daily} (which is expected to live on a shorter-retention rule). A + * broad rule {@code patterns=["*"], excludePatterns=["user_*"]} loads every projection except those handled by a + * more specific {@code user_*} rule elsewhere in the cascade. + */ +public class WildcardProjectionPartialLoadMatcher extends ProjectionPartialLoadMatcher +{ + public static final String TYPE = "globProjection"; + + private final List<String> patterns; + private final List<String> excludePatterns; + + @JsonCreator + public WildcardProjectionPartialLoadMatcher( + @JsonProperty("patterns") List<String> patterns, + @JsonProperty("excludePatterns") @Nullable List<String> excludePatterns + ) + { + if (patterns == null || patterns.isEmpty()) { + throw InvalidInput.exception("patterns must not be null or empty for globProjection matcher"); + } + this.patterns = List.copyOf(patterns); + this.excludePatterns = excludePatterns == null ? List.of() : List.copyOf(excludePatterns); + } + + @JsonProperty + public List<String> getPatterns() + { + return patterns; + } + + @JsonProperty + public List<String> getExcludePatterns() + { + return excludePatterns; + } + + @Override + protected List<String> resolveProjectionNames(DataSegment segment) + { + final List<String> segmentProjections = segment.getProjections(); + if (segmentProjections == null || segmentProjections.isEmpty()) { + return Collections.emptyList(); + } + final TreeSet<String> matched = new TreeSet<>(); + for (String name : segmentProjections) { + if (matchesAny(name, excludePatterns)) { + continue; + } + if (matchesAny(name, patterns)) { + matched.add(name); + } + } + return new ArrayList<>(matched); + } + + private static boolean matchesAny(String name, List<String> patterns) + { + for (String pattern : patterns) { + if (FilenameUtils.wildcardMatch(name, pattern)) { Review Comment: How does this treat `\*`? I wonder if it escapes the `*` or if it looks for a literal `\` followed by anything. It's probaly not likely that a projection name will include `*` or `?`, but it would be nice to support escaping anyway, in case they do. ########## server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodPartialLoadRule.java: ########## @@ -0,0 +1,104 @@ +/* + * 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.rules; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; +import org.joda.time.DateTime; +import org.joda.time.Interval; +import org.joda.time.Period; + +import javax.annotation.Nullable; +import java.util.Map; +import java.util.Objects; + +/** + * Period-based variant of {@link PartialLoadRule}. Mirrors {@link PeriodLoadRule} for time-window semantics and layers + * on {@link PartialLoadMatcher} for selection. + */ +public class PeriodPartialLoadRule extends PartialLoadRule +{ + public static final String TYPE = "loadPartialByPeriod"; + + private final Period period; + private final boolean includeFuture; + + @JsonCreator + public PeriodPartialLoadRule( + @JsonProperty("period") Period period, + @JsonProperty("includeFuture") @Nullable Boolean includeFuture, + @JsonProperty("tieredReplicants") Map<String, Integer> tieredReplicants, + @JsonProperty("useDefaultTierForNull") @Nullable Boolean useDefaultTierForNull, + @JsonProperty("matcher") PartialLoadMatcher matcher, + @JsonProperty("onCannotMatch") @Nullable CannotMatchBehavior onCannotMatch + ) + { + super(tieredReplicants, useDefaultTierForNull, matcher, onCannotMatch); + this.period = period; + this.includeFuture = includeFuture == null ? PeriodLoadRule.DEFAULT_INCLUDE_FUTURE : includeFuture; + } + + @Override + @JsonProperty + public String getType() + { + return TYPE; + } + + @JsonProperty + public Period getPeriod() + { + return period; + } + + @JsonProperty + public boolean isIncludeFuture() + { + return includeFuture; + } + + @Override + public boolean appliesTo(Interval interval, DateTime referenceTimestamp) + { + return Rules.eligibleForLoad(period, interval, referenceTimestamp, includeFuture); Review Comment: I wonder if anything weird will happen because `appliesTo(segment.getInterval(), refTs)` doesn't always match `appliesTo(segment, refTs)` like it does with the other rules. This overload is used in two places: - `DataSourcesResource#isHandOffComplete`, where it's used to short-circuit handoff checking for segments that no load rule applies to. I suppose the risk here is that we're in a situation where really no rule matches (because the segment doesn't have any desired projections for a partial-rule, and there's no other rule to fall back to), but the handoff checker thinks the partial-rule would match, and so handoff times out. This seems unlikely to happen— probably in a real cluster there would be another rule to fall back on— but is worth a comment somewhere acknowledging it's a risk. Possibly right here. - `TieredBrokerHostSelector`, where it's used to find the rule that will drive which Broker handles a request when `druid.router.tierToBrokerMap` is set. This is probably fine: in some configurations of load rules it might change which broker gets a query, but probably the behavior is defensible given that the routing isn't projection-aware. -- 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]
