Jackie-Jiang commented on code in PR #18486:
URL: https://github.com/apache/pinot/pull/18486#discussion_r4499111668
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -370,6 +370,17 @@ public static long getRandomInitialDelayInSeconds() {
// Amount of the time the segment can take from the beginning of upload to
the end of upload. Used when parallel push
// protection is enabled. If the upload does not finish within the timeout,
next upload can override the previous one.
public static final String SEGMENT_UPLOAD_TIMEOUT_IN_MILLIS =
"controller.segment.upload.timeoutInMillis";
+ /// If true, the controller may reject single-segment uploads with HTTP 403
when segment data end is past the table
+ /// data retention window (see {@link
org.apache.pinot.controller.api.upload.SegmentValidationUtils}).
+ ///
+ /// This also applies to minion task uploads that push through `POST
/segments`: always for Purge, UpsertCompaction
+ /// and RefreshSegment, and for RealtimeToOffline, MergeRollup and
UpsertCompactMerge unless `batchSegmentUpload` is
+ /// on.
+ /// A rejected RealtimeToOffline upload does not advance its watermark, so
the task retries the same window until the
+ /// source segments age out (indefinitely with the hybrid retention
strategy, since the time boundary cannot advance).
+ /// Review task schedules and buffer periods against table retention before
enabling.
+ public static final String SEGMENT_UPLOAD_REJECT_OUT_OF_RETENTION_ENABLED =
+ "controller.segment.upload.rejectOutOfRetention.enabled";
Review Comment:
Suggestion: rename this to
`controller.segment.upload.reject.out.of.retention`, retaining the `false`
default. `reject` already expresses the boolean action, so `.enabled` is
redundant. `ControllerConf` mixes styles, but lowercase, dot-separated words
fit keys such as `controller.enable.storage.quota.check`. Rename the constants
to `SEGMENT_UPLOAD_REJECT_OUT_OF_RETENTION` and
`DEFAULT_SEGMENT_UPLOAD_REJECT_OUT_OF_RETENTION`, and update the accessors,
references, and documentation accordingly. Since this key is new and
unreleased, a direct rename does not need a compatibility alias.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/RetentionUtils.java:
##########
@@ -69,4 +109,43 @@ public static boolean isPurgeable(String tableNameWithType,
SegmentZKMetadata se
}
return false;
}
+
+ /// Parses {@link
SegmentsValidationAndRetentionConfig#getRetentionTimeUnit()} and
+ /// {@link SegmentsValidationAndRetentionConfig#getRetentionTimeValue()}
into table data retention duration in
+ /// milliseconds (same interpretation as controller time-based retention).
+ ///
+ /// @return millis, or empty if unset or not parseable
+ public static OptionalLong parseTableDataRetentionMillis(
+ @Nullable SegmentsValidationAndRetentionConfig validationConfig) {
+ if (validationConfig == null) {
+ return OptionalLong.empty();
+ }
+ String retentionTimeUnit = validationConfig.getRetentionTimeUnit();
+ String retentionTimeValue = validationConfig.getRetentionTimeValue();
+ if (StringUtils.isEmpty(retentionTimeUnit) ||
StringUtils.isEmpty(retentionTimeValue)) {
+ return OptionalLong.empty();
+ }
+ try {
+ return
OptionalLong.of(TimeUnit.valueOf(retentionTimeUnit.toUpperCase(Locale.ROOT))
+ .toMillis(Long.parseLong(retentionTimeValue)));
+ } catch (Exception e) {
+ LOGGER.debug("Failed to parse table data retention: unit='{}'
value='{}'", retentionTimeUnit, retentionTimeValue,
+ e);
+ return OptionalLong.empty();
+ }
+ }
+
+ /// Returns segment end time in epoch millis from on-disk {@link
SegmentMetadata} for retention comparison, using
+ /// {@link SegmentMetadata#getTimeUnit()} and {@link
SegmentMetadata#getEndTime()} (same raw fields minion task
+ /// executors pass when configuring generation from segment files). When
`timeUnit` is non-null and raw end time is
+ /// not {@link Long#MIN_VALUE}, returns `timeUnit.toMillis(endTime)`;
otherwise returns the raw end time (typically
+ /// `Long.MIN_VALUE` when unset).
+ public static long getSegmentMetadataEndTimeMillis(SegmentMetadata
segmentMetadata) {
Review Comment:
Suggestion: make `getSegmentMetadataEndTimeMillis()` private. Outside tests,
its only caller is within `RetentionUtils`, so it does not need to become
public API. Cover conversion through retention outcomes and remove tests that
assert which metadata getters are called; that preserves behavioral coverage
without locking down the implementation.
##########
pinot-common/src/test/java/org/apache/pinot/common/utils/RetentionUtilsTest.java:
##########
@@ -47,6 +61,77 @@ private static SegmentZKMetadata
makeSegmentWithCreationTime(long endTimeMs, lon
return segment;
}
+ private static SegmentMetadata mockSegmentMetadataMillis(long endTimeRaw,
long indexCreationTimeMs) {
+ SegmentMetadata metadata = Mockito.mock(SegmentMetadata.class);
Review Comment:
Nit: use static imports for the new Mockito and Assert calls, as required by
the repository conventions. The same cleanup applies to the new qualified
assertion calls in `ControllerConfTest` and `SegmentUploadIntegrationTest`.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/upload/SegmentValidationUtils.java:
##########
@@ -103,6 +108,59 @@ private static String getPartitionColumn(TableConfig
tableConfig) {
return null;
}
+ /// When `controller.segment.upload.rejectOutOfRetention.enabled` is true on
the controller, rejects the upload if
+ /// the segment's data end time (from segment file metadata) is past the
table's `retentionTimeValue` /
+ /// `retentionTimeUnit` window. This matches the retention manager's
time-based boundary. It does not model the
+ /// hybrid retention strategy
(`controller.enable.hybrid.table.retention.strategy`), under which the REALTIME
side of
+ /// a hybrid table is purged by the offline time boundary instead of the
retention window. The two boundaries are
+ /// independent there, so this check can be either stricter or looser than
what the retention manager purges.
+ /// **Index creation time is not** used as a fallback here: it can reflect
upstream segment timestamps (for example
+ /// upsert compaction) and is a poor proxy for the data window at upload
time.
+ ///
+ /// Rejection uses HTTP 403 so callers that enable the controller flag can
treat the response as a hard failure.
+ /// Clients that do not enable the flag see no behavior change.
+ ///
+ /// Controller wiring: {@link
org.apache.pinot.controller.api.resources.PinotSegmentUploadDownloadRestletResource}
+ /// invokes this for single-segment upload only. METADATA batch upload
(`POST /segments/batchUpload`) and
+ /// reingested-segment upload do not call it.
+ ///
+ /// Minion task uploads are not exempt. Purge, UpsertCompaction and
RefreshSegment always push through this
+ /// single-segment endpoint. RealtimeToOffline, MergeRollup and
UpsertCompactMerge do too unless `batchSegmentUpload`
+ /// is enabled for the task. A rejected RealtimeToOffline upload does not
advance the task watermark, so the same
+ /// window is retried on every run until the source segments age out.
+ ///
+ /// For tables where the retention manager does not apply time-based
retention to completed segments (offline tables
+ /// whose batch ingestion type is not `APPEND`), this method returns without
evaluating retention.
+ public static void rejectUploadIfOutOfRetention(SegmentMetadata
segmentMetadata, TableConfig tableConfig,
+ long currentTimeMs, boolean controllerRejectOutOfRetentionEnabled,
+ @Nullable ControllerMetrics controllerMetrics) {
+ if (!controllerRejectOutOfRetentionEnabled) {
+ return;
+ }
+ SegmentsValidationAndRetentionConfig validationConfig =
tableConfig.getValidationConfig();
+ if (validationConfig == null) {
Review Comment:
Suggestion: rely on `TableConfig`'s non-null validation-config invariant
here. Both its constructor and `setValidationConfig()` reject null, and all
production callers of the new parser obtain the config from `TableConfig`. This
guard, the parser's `@Nullable`/null handling, and the mock-only null-config
tests can be removed. Keep coverage for a non-null config with missing or
invalid retention fields.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/RetentionUtils.java:
##########
@@ -49,18 +69,38 @@ private RetentionUtils() {
/// @return true if the segment is past the retention boundary, false
otherwise
public static boolean isPurgeable(String tableNameWithType,
SegmentZKMetadata segmentZKMetadata, long retentionMs,
long currentTimeMs, boolean useCreationTimeFallback) {
- String segmentName = segmentZKMetadata.getSegmentName();
- long endTimeMs = segmentZKMetadata.getEndTimeMs();
+ return isPurgeableInternal(segmentZKMetadata.getEndTimeMs(),
segmentZKMetadata.getCreationTime(), retentionMs,
+ currentTimeMs, useCreationTimeFallback, tableNameWithType,
segmentZKMetadata.getSegmentName());
+ }
+
+ /// Whether segment file metadata is past the retention window, using end
time from
+ /// {@link #getSegmentMetadataEndTimeMillis(SegmentMetadata)} and
+ /// {@link SegmentMetadata#getIndexCreationTime()} as the optional fallback
clock. Invalid-end paths emit the same
+ /// style of debug/warn lines as the ZK overload, using
+ /// {@link SegmentMetadata#getName()} for the segment in log messages.
+ ///
+ /// - If end time is valid: purgeable when `currentTimeMs - endTimeMs >
retentionMs`.
+ /// - If end time is invalid and `useCreationTimeFallback` is true and index
creation time is valid: purgeable when
+ /// `currentTimeMs - creationTimeMs > retentionMs`.
+ /// - Otherwise: not purgeable (fail-open).
+ public static boolean isPurgeable(String tableNameWithType, SegmentMetadata
segmentMetadata, long retentionMs,
+ long currentTimeMs, boolean useCreationTimeFallback) {
Review Comment:
Suggestion: remove `useCreationTimeFallback` from this new `SegmentMetadata`
overload and avoid reading `getIndexCreationTime()`. Its only production caller
passes `false`, and the upload contract explicitly excludes index-creation-time
fallback. Keeping the option adds API and tests for behavior this feature does
not use. Preserve creation-time fallback in the existing `SegmentZKMetadata`
overload.
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/retention/TableConfigRetentionUtilsTest.java:
##########
@@ -0,0 +1,91 @@
+/**
+ * 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.pinot.controller.helix.core.retention;
+
+import java.util.concurrent.TimeUnit;
+import org.apache.pinot.common.metadata.segment.SegmentZKMetadata;
+import
org.apache.pinot.controller.helix.core.retention.strategy.RetentionStrategy;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.utils.builder.TableConfigBuilder;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertNotNull;
+import static org.testng.Assert.assertNull;
+import static org.testng.Assert.assertTrue;
+
+
+public class TableConfigRetentionUtilsTest {
Review Comment:
Nit: add class-level Javadoc describing the retention-strategy behavior
covered by this new test class, per the repository convention for new classes.
--
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]
