roryqi commented on code in PR #13801: URL: https://github.com/apache/gravitino/pull/13801#discussion_r4268358608
########## core/src/main/java/org/apache/gravitino/maintenance/policy/MaintenanceMinIntervalConfigs.java: ########## @@ -0,0 +1,152 @@ +/* + * 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 javax.annotation.Nullable; +import org.apache.gravitino.Config; +import org.apache.gravitino.config.ConfigBuilder; +import org.apache.gravitino.config.ConfigConstants; +import org.apache.gravitino.config.ConfigEntry; +import org.apache.gravitino.policy.Policy; + +/** + * Server config entries for per-type {@code minIntervalMs} cooldowns in {@code gravitino.conf}. + * + * <p>Used when a maintenance policy omits {@code minIntervalMs}. Resolution order is policy content + * → these server keys → the code defaults below. + */ +public final class MaintenanceMinIntervalConfigs { + + /** Default cooldown for compaction / manifest-rewrite / snapshot-expiry (1 hour). */ + public static final long DEFAULT_HOURLY_MIN_INTERVAL_MS = 3_600_000L; + + /** Default cooldown for orphan-cleanup (1 day). */ + public static final long DEFAULT_ORPHAN_CLEANUP_MIN_INTERVAL_MS = 86_400_000L; + + /** Config key for compaction cooldown. */ + public static final String COMPACTION_MIN_INTERVAL_MS_KEY = + "gravitino.maintenance.compaction.minIntervalMs"; + + /** Config key for manifest-rewrite cooldown. */ + public static final String MANIFEST_REWRITE_MIN_INTERVAL_MS_KEY = + "gravitino.maintenance.manifest-rewrite.minIntervalMs"; + + /** Config key for snapshot-expiry cooldown. */ + public static final String SNAPSHOT_EXPIRY_MIN_INTERVAL_MS_KEY = + "gravitino.maintenance.snapshot-expiry.minIntervalMs"; + + /** Config key for orphan-cleanup cooldown. */ + public static final String ORPHAN_CLEANUP_MIN_INTERVAL_MS_KEY = + "gravitino.maintenance.orphan-cleanup.minIntervalMs"; + + /** Cooldown between finished compaction runs when the policy omits {@code minIntervalMs}. */ + public static final ConfigEntry<Long> COMPACTION_MIN_INTERVAL_MS = + new ConfigBuilder(COMPACTION_MIN_INTERVAL_MS_KEY) + .doc( + "Minimum interval in milliseconds between finished compaction runs when the " + + "policy omits minIntervalMs") + .version(ConfigConstants.VERSION_2_0_0) + .longConf() + .checkValue(v -> v != null && v >= 0, "minIntervalMs must be >= 0") + .createWithDefault(DEFAULT_HOURLY_MIN_INTERVAL_MS); + + /** + * Cooldown between finished manifest-rewrite runs when the policy omits {@code minIntervalMs}. + */ + public static final ConfigEntry<Long> MANIFEST_REWRITE_MIN_INTERVAL_MS = + new ConfigBuilder(MANIFEST_REWRITE_MIN_INTERVAL_MS_KEY) + .doc( + "Minimum interval in milliseconds between finished manifest-rewrite runs when the " + + "policy omits minIntervalMs") + .version(ConfigConstants.VERSION_2_0_0) + .longConf() + .checkValue(v -> v != null && v >= 0, "minIntervalMs must be >= 0") + .createWithDefault(DEFAULT_HOURLY_MIN_INTERVAL_MS); + + /** Cooldown between finished snapshot-expiry runs when the policy omits {@code minIntervalMs}. */ + public static final ConfigEntry<Long> SNAPSHOT_EXPIRY_MIN_INTERVAL_MS = + new ConfigBuilder(SNAPSHOT_EXPIRY_MIN_INTERVAL_MS_KEY) + .doc( + "Minimum interval in milliseconds between finished snapshot-expiry runs when the " + + "policy omits minIntervalMs") + .version(ConfigConstants.VERSION_2_0_0) + .longConf() + .checkValue(v -> v != null && v >= 0, "minIntervalMs must be >= 0") + .createWithDefault(DEFAULT_HOURLY_MIN_INTERVAL_MS); + + /** Cooldown between finished orphan-cleanup runs when the policy omits {@code minIntervalMs}. */ + public static final ConfigEntry<Long> ORPHAN_CLEANUP_MIN_INTERVAL_MS = + new ConfigBuilder(ORPHAN_CLEANUP_MIN_INTERVAL_MS_KEY) + .doc( + "Minimum interval in milliseconds between finished orphan-cleanup runs when the " + + "policy omits minIntervalMs") + .version(ConfigConstants.VERSION_2_0_0) + .longConf() + .checkValue(v -> v != null && v >= 0, "minIntervalMs must be >= 0") + .createWithDefault(DEFAULT_ORPHAN_CLEANUP_MIN_INTERVAL_MS); + + private MaintenanceMinIntervalConfigs() {} + + /** + * Returns the {@code minIntervalMs} config entry for a built-in maintenance policy type. + * + * @param builtInType built-in policy type + * @return config entry + */ + public static ConfigEntry<Long> forBuiltInType(Policy.BuiltInType builtInType) { + Preconditions.checkNotNull(builtInType, "builtInType cannot be null"); + switch (builtInType) { Review Comment: Removing `TableMaintenanceTaskType` does not remove the duplicate type model; this switch recreates the same 1:1 mapping between `Policy.BuiltInType`, maintenance config keys, and defaults. The mapping is also repeated by strategy values and job-template values in each content class. Please use one core-side maintenance definition registry keyed by `Policy.BuiltInType`, so the policy type remains the single identity and adding a maintenance policy does not require parallel switches and constants across layers. ########## api/src/main/java/org/apache/gravitino/policy/PolicyContent.java: ########## @@ -47,13 +49,76 @@ default Map<String, Object> rules() { throw new UnsupportedOperationException("Does support get all rules."); } + /** + * Optional cron expression that schedules automated policy evaluation. + * + * <p>Wire form is a plain string, e.g. {@code "timeTrigger": "0 0 2 * * ?"}. Cron syntax is not + * validated on the policy API; the scheduler registration path parses it. + * + * @return cron expression, or {@code null} when unset + */ + @Nullable + default String timeTrigger() { Review Comment: These execution fields are not capabilities shared by every `PolicyContent`. Row-filter, column-mask, and custom policy contents do not have meaningful time triggers, commit triggers, cooldowns, or job/update options. Defaulting them to null/false/empty also makes “unsupported by this policy type” indistinguishable from “supported but unset”. Please keep `PolicyContent` limited to universal policy behavior and put these fields on a maintenance-specific subtype or narrower capability interfaces. ########## api/src/main/java/org/apache/gravitino/policy/PolicyContent.java: ########## @@ -47,13 +49,76 @@ default Map<String, Object> rules() { throw new UnsupportedOperationException("Does support get all rules."); } + /** + * Optional cron expression that schedules automated policy evaluation. + * + * <p>Wire form is a plain string, e.g. {@code "timeTrigger": "0 0 2 * * ?"}. Cron syntax is not + * validated on the policy API; the scheduler registration path parses it. + * + * @return cron expression, or {@code null} when unset + */ + @Nullable + default String timeTrigger() { + return null; + } + + /** + * Optional flag that evaluates the policy after an Iceberg commit. + * + * <p>Wire form is a plain boolean, e.g. {@code "commitTrigger": true}. {@code null} or {@code + * false} means unset / disabled. + * + * @return {@code true} when commit-triggered evaluation is enabled, otherwise {@code null} or + * {@code false} + */ + @Nullable + default Boolean commitTrigger() { + return null; + } + + /** + * Optional cooldown between finished automated runs for this policy. + * + * @return minimum interval in milliseconds, or {@code null} when unset + */ + @Nullable + default Long minIntervalMs() { + return null; + } + + /** + * Optional job / client options carried in policy content (non-secret). + * + * @return job options; never {@code null} + */ + default Map<String, String> jobOptions() { + return Collections.emptyMap(); + } + + /** + * Optional updater / control-plane options carried in policy content (non-secret). + * + * @return update options; never {@code null} + */ + default Map<String, String> updateOptions() { + return Collections.emptyMap(); + } + /** * Validates the policy content. * + * <p>Checks {@link #supportedObjectTypes()} and the shared execution peer fields ({@link + * #timeTrigger()}, {@link #commitTrigger()}, {@link #minIntervalMs()}, {@link #jobOptions()}, + * {@link #updateOptions()}). Implementations that override this method must call {@code + * PolicyContent.super.validate()} (or equivalent) so peer validation stays on the persistence + * path. + * * @throws IllegalArgumentException if the content is invalid. */ default void validate() throws IllegalArgumentException { Preconditions.checkArgument( CollectionUtils.isNotEmpty(supportedObjectTypes()), "supportedObjectTypes cannot be empty"); + PolicyExecutionFields.validate( Review Comment: This validation is both too broad and too weak. It runs TMS execution validation for every policy content, but it cannot enforce type-specific invariants such as rejecting commit trigger for orphan cleanup, requiring a supported trigger combination, validating cron with the runtime parser, or constraining option keys. Those checks can also be bypassed through non-DTO creation paths. Please keep universal validation here and perform maintenance validation on the maintenance subtype/service create-alter path, using the actual built-in type and runtime contracts. -- 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]
