lasdf1234 commented on code in PR #13801: URL: https://github.com/apache/gravitino/pull/13801#discussion_r4268812523
########## 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: Agreed — replaced the switch with a `MaintenancePolicyDefinitions` registry keyed only by `Policy.BuiltInType` (see `MaintenancePolicyDefinition` / `MaintenancePolicyDefinitions`). `forBuiltInType` now looks up that registry instead of maintaining a parallel mapping. ########## 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: Agreed. Peers are no longer on universal `PolicyContent`. They live on `AbstractExecutablePolicyContent`; the four Iceberg maintenance contents extend it and attach peers via `withExecutionFields`. Row-filter / column-mask / custom stay without those accessors. ########## 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: Agreed on the layering. `PolicyContent.validate()` is universal-only (`supportedObjectTypes`). Shared peer checks run in `AbstractExecutablePolicyContent.validate()`. Type-specific rules that already fit on the content stay there (e.g. orphan rejects `commitTrigger`). Cron runtime parse / sensitive keys / richer BuiltInType contracts remain follow-ups on the create-alter / scheduler path. -- 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]
