roryqi commented on code in PR #13814: URL: https://github.com/apache/gravitino/pull/13814#discussion_r4268359658
########## api/src/main/java/org/apache/gravitino/policy/AbstractExecutablePolicyContent.java: ########## @@ -0,0 +1,144 @@ +/* + * 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.policy; + +import java.util.Map; +import java.util.Objects; +import javax.annotation.Nullable; +import org.apache.commons.lang3.StringUtils; + +/** + * Base for built-in policy contents that may carry optional execution peer fields. + * + * <p>Subclasses keep strategy-only constructors. Callers attach execution fields through {@link + * #withExecutionFields}. + */ +public abstract class AbstractExecutablePolicyContent implements PolicyContent { + + @Nullable private String timeTrigger; + @Nullable private Boolean commitTrigger; + @Nullable private Long minIntervalMs; + private Map<String, String> jobOptions = PolicyExecutionFields.copyOptions(null); + private Map<String, String> updateOptions = PolicyExecutionFields.copyOptions(null); + + /** Creates content with unset execution fields. */ + protected AbstractExecutablePolicyContent() {} + + /** + * Attaches optional execution fields. Strategy subclasses do not take these in constructors. + * + * @param timeTrigger optional cron expression + * @param commitTrigger optional commit-trigger flag + * @param minIntervalMs optional cooldown ms + * @param jobOptions optional job options + * @param updateOptions optional update options + * @param <T> concrete content type + * @return this instance + */ + @SuppressWarnings("unchecked") + public <T extends AbstractExecutablePolicyContent> T withExecutionFields( Review Comment: This makes policy content mutable after construction and uses an unchecked generic self-cast. It permits invalid intermediate states, is unsafe if the content has already participated in `equals`/`hashCode`, and requires every subclass to remember the execution fields in equality and string methods. Policy content is a persisted domain value and should be immutable. Please use an immutable execution-settings value object and return a newly constructed content, or require the settings in a builder/constructor. ########## common/src/main/java/org/apache/gravitino/dto/policy/PolicyContentDTO.java: ########## @@ -114,6 +115,26 @@ class IcebergCompactionContentDTO implements PolicyContentDTO { @JsonProperty("rewriteOptions") private Map<String, String> rewriteOptions; + @JsonProperty("timeTrigger") + @Nullable + private String timeTrigger; Review Comment: The same five execution fields, accessors, null/default handling, validation, and domain conversion are repeated across every maintenance DTO. This will require every future field change to be synchronized across all DTO classes. Please extract a maintenance execution DTO/base transport component and centralize the mapping. This should remain maintenance-specific rather than becoming another common `PolicyContentDTO` contract. -- 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]
