capistrant commented on code in PR #18939: URL: https://github.com/apache/druid/pull/18939#discussion_r2825624511
########## server/src/main/java/org/apache/druid/server/compaction/AbstractReindexingRule.java: ########## @@ -0,0 +1,167 @@ +/* + * 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.compaction; + +import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.druid.java.util.common.DateTimes; +import org.apache.druid.java.util.common.logger.Logger; +import org.joda.time.DateTime; +import org.joda.time.Interval; +import org.joda.time.Period; + +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import java.util.Objects; + +/** + * Base implementation for reindexing rules that apply based on data age thresholds. + * <p> + * Provides period-based applicability logic: a rule with {@link AbstractReindexingRule#olderThan} of P7D applies to + * data older than 7 days as compared to the time of evaluation. Subclasses define specific reindexing configuration + * (granularity, filters, tuning, etc.) and whether multiple rules can combine (additive vs non-additive). + * <p> + * The {@link #appliesTo(Interval, DateTime)} method determines if an interval is fully, + * partially, or not covered by this rule's threshold, enabling cascading reindexing + * strategies where different rules apply to different age tiers of data. + */ +public abstract class AbstractReindexingRule implements ReindexingRule +{ + private static final Logger LOG = new Logger(AbstractReindexingRule.class); + + private final String id; + private final String description; + private final Period olderThan; + + public AbstractReindexingRule( + @Nonnull String id, + @Nullable String description, + @Nonnull Period olderThan + ) + { + this.id = Objects.requireNonNull(id, "id cannot be null"); + this.description = description; + this.olderThan = Objects.requireNonNull(olderThan, "olderThan period cannot be null"); + + validatePeriodIsPositive(olderThan); + } + + /** + * Validates that a period represents a positive duration. + * <p> + * For periods with precise units (days, hours, minutes, seconds), validates by converting + * to a standard duration. For periods with variable-length units (months, years), validates + * that at least one component is positive, since these cannot be converted to a precise duration. + * + * @param period the period to validate + * @throws IllegalArgumentException if the period is not positive + */ + private static void validatePeriodIsPositive(Period period) + { + if (hasMonthsOrYears(period)) { + if (!isPeriodPositive(period)) { + throw new IllegalArgumentException("period must be positive. Supplied period: " + period); + } + } else { + if (period.toStandardDuration().getMillis() <= 0) { + throw new IllegalArgumentException("period must be positive. Supplied period: " + period); + } + } + } + + /** + * Checks if a period with variable-length components (months/years) is positive. + * + * @param period the period to check + * @return true if any component is positive and no components are negative + */ + private static boolean isPeriodPositive(Period period) Review Comment: ya. I think I accidentally pulled it from the description that period of 0 and handling of future data need to be addressed. Perhaps this PR warrants supporting P0D. but future data should be left for follow on if needed -- 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]
