roryqi commented on code in PR #13817: URL: https://github.com/apache/gravitino/pull/13817#discussion_r4294187423
########## core/src/main/java/org/apache/gravitino/maintenance/policy/NearestMaintenancePolicyResolver.java: ########## @@ -0,0 +1,123 @@ +/* + * 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.gravitino.maintenance.policy; + +import com.google.common.base.Preconditions; +import java.util.ArrayList; +import java.util.Comparator; +import java.util.List; +import java.util.Optional; +import org.apache.gravitino.MetadataObject; +import org.apache.gravitino.meta.PolicyEntity; +import org.apache.gravitino.policy.Policy; +import org.apache.gravitino.policy.PolicyDispatcher; +import org.apache.gravitino.utils.MetadataObjectUtil; + +/** + * Selects attached maintenance policies of a given built-in type for a metadata object. + * + * <p>Resolution walks the metadata object and its ancestors from nearest to outermost (for a table: + * table → schema → catalog). At each level, only policies attached on that object are considered + * ({@code inherited == false} from {@link PolicyDispatcher#listPolicyInfosForMetadataObject}). + * Among matches at the same level, the policy with the lexicographically smallest name wins. + * + * <p>{@link #resolveNearest} returns the first match in that walk. {@link #resolveDirectChain} + * returns every level that has a direct match so callers can merge {@code jobOptions} / {@code + * updaterOptions} across the hierarchy. + */ +public class NearestMaintenancePolicyResolver { + + private final PolicyDispatcher policyDispatcher; + + /** + * Creates a resolver. + * + * @param policyDispatcher policy dispatcher + */ + public NearestMaintenancePolicyResolver(PolicyDispatcher policyDispatcher) { + this.policyDispatcher = Preconditions.checkNotNull(policyDispatcher, "policyDispatcher"); + } + + /** + * Returns the nearest enabled policy entity for the built-in maintenance policy type. + * + * @param metalake metalake name + * @param metadataObject metadata object (typically a table) + * @param builtInType built-in maintenance policy type + * @return matching policy when found + */ + public Optional<PolicyEntity> resolveNearest( + String metalake, MetadataObject metadataObject, Policy.BuiltInType builtInType) { + List<PolicyEntity> chain = resolveDirectChain(metalake, metadataObject, builtInType); + return chain.isEmpty() ? Optional.empty() : Optional.of(chain.get(0)); + } + + /** + * Returns direct matching policies from nearest to outermost (at most one per level). + * + * <p>Used when merging {@code jobOptions} / {@code updaterOptions} across the hierarchy while + * triggers and {@code minIntervalMs} still come from the nearest policy only. + * + * @param metalake metalake name + * @param metadataObject metadata object (typically a table) + * @param builtInType built-in maintenance policy type + * @return nearest-first chain of direct matches + */ + public List<PolicyEntity> resolveDirectChain( + String metalake, MetadataObject metadataObject, Policy.BuiltInType builtInType) { + Preconditions.checkNotNull(metalake, "metalake cannot be null"); + MetadataObjectUtil.checkMetadataObject(metalake, metadataObject); + Preconditions.checkNotNull(builtInType, "builtInType cannot be null"); + Preconditions.checkArgument( + builtInType != Policy.BuiltInType.CUSTOM, "builtInType must be a built-in type"); + + List<PolicyEntity> chain = new ArrayList<>(); + for (MetadataObject level : resolutionOrder(metadataObject)) { Review Comment: Why do we need another policy resolution pass here? `PolicyDispatcher.listPolicyInfosForMetadataObject()` already resolves the effective policies for the target object, including tag inheritance/override, selector matching, disabled-policy filtering, and deduplication. Walking the ancestors and resolving each one independently can change the semantics. For example, a table-level tag value may override a schema-level value and make the schema policy inapplicable, but resolving the schema separately adds that policy back. Could we reuse the existing object-policy resolution result directly? If the scheduler additionally needs the exact source level, that provenance should be exposed by `ObjectPolicyResolver` rather than re-resolving every ancestor. ########## core/src/main/java/org/apache/gravitino/maintenance/policy/EffectiveTableMaintenancePolicy.java: ########## @@ -0,0 +1,206 @@ +/* + * 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.gravitino.maintenance.policy; + +import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableMap; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import javax.annotation.Nullable; +import org.apache.gravitino.Config; +import org.apache.gravitino.MetadataObject; +import org.apache.gravitino.meta.PolicyEntity; +import org.apache.gravitino.policy.ExecutablePolicyContent; +import org.apache.gravitino.policy.Policy; +import org.apache.gravitino.policy.PolicyContent; +import org.apache.gravitino.policy.PolicyExecutionSpec; + +/** + * Effective scheduling / submit view for a table and built-in maintenance policy type. + * + * <p>Carries the nearest policy for triggers and resolved {@code minIntervalMs}, plus {@code + * jobOptions} / {@code updaterOptions} merged across the metadata hierarchy (catalog → schema → + * table). Missing keys on a nearer attachment are filled from outer levels; the same key on a + * nearer policy overrides an outer value. + */ +public final class EffectiveTableMaintenancePolicy { + + private final PolicyEntity policy; + private final Policy.BuiltInType builtInType; + @Nullable private final String timeTrigger; + @Nullable private final Boolean commitTrigger; + private final long minIntervalMs; + private final Map<String, String> jobOptions; + private final Map<String, String> updaterOptions; + + /** + * Creates an effective policy view. + * + * @param policy nearest policy entity + * @param builtInType built-in maintenance policy type + * @param timeTrigger cron expression from the nearest policy content, or null when unset + * @param commitTrigger commit-trigger flag from the nearest policy content, or null when unset + * @param minIntervalMs resolved minimum interval + * @param jobOptions hierarchy-merged job options + * @param updaterOptions hierarchy-merged updater options + */ + public EffectiveTableMaintenancePolicy( + PolicyEntity policy, + Policy.BuiltInType builtInType, + @Nullable String timeTrigger, + @Nullable Boolean commitTrigger, + long minIntervalMs, + Map<String, String> jobOptions, + Map<String, String> updaterOptions) { + this.policy = Preconditions.checkNotNull(policy, "policy"); + this.builtInType = Preconditions.checkNotNull(builtInType, "builtInType"); + this.timeTrigger = timeTrigger; + this.commitTrigger = commitTrigger; + this.minIntervalMs = minIntervalMs; + this.jobOptions = copyOptions(jobOptions); + this.updaterOptions = copyOptions(updaterOptions); + } + + /** + * @return nearest policy entity + */ + public PolicyEntity policy() { + return policy; + } + + /** + * @return built-in maintenance policy type + */ + public Policy.BuiltInType builtInType() { + return builtInType; + } + + /** + * @return cron expression from the nearest policy content, or null when unset + */ + @Nullable + public String timeTrigger() { + return timeTrigger; + } + + /** + * @return commit-trigger flag from the nearest policy content, or null when unset + */ + @Nullable + public Boolean commitTrigger() { + return commitTrigger; + } + + /** + * @return resolved minimum interval between runs + */ + public long minIntervalMs() { + return minIntervalMs; + } + + /** + * @return hierarchy-merged job options; never {@code null} + */ + public Map<String, String> jobOptions() { + return jobOptions; + } + + /** + * @return hierarchy-merged updater options; never {@code null} + */ + public Map<String, String> updaterOptions() { + return updaterOptions; + } + + /** + * Resolves effective TMS policy for a metadata object. + * + * <p>{@code timeTrigger}, {@code commitTrigger}, and {@code minIntervalMs} come from the nearest + * matching policy. {@code jobOptions} / {@code updaterOptions} are the union of direct matches + * along catalog → schema → table (nearer keys win). + * + * @param resolver nearest-policy resolver + * @param metalake metalake name + * @param metadataObject metadata object + * @param builtInType built-in maintenance policy type + * @param config gravitino server configuration + * @return effective policy when a matching policy exists + */ + public static Optional<EffectiveTableMaintenancePolicy> resolve( + NearestMaintenancePolicyResolver resolver, + String metalake, + MetadataObject metadataObject, + Policy.BuiltInType builtInType, + Config config) { + List<PolicyEntity> chain = resolver.resolveDirectChain(metalake, metadataObject, builtInType); Review Comment: Do we need `EffectiveTableMaintenancePolicy` here? `PolicyEntity` already carries the selected policy and its `PolicyExecutionSpec`. The only additional derived value appears to be the server-defaulted `minIntervalMs`, which can be resolved directly through `MaintenanceMinIntervalConfigs.resolve()`. The wrapper currently duplicates most execution fields and also introduces cross-policy option merging, even though the design says the nearest policy wins as a whole. Could we keep `PolicyEntity` as the runtime source of truth and resolve only `minIntervalMs` at the scheduler boundary? If a normalized runtime value object becomes necessary, it should probably contain only the genuinely derived execution values rather than duplicate policy selection and hierarchy semantics. -- 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]
