shuturmurgh commented on code in PR #18486:
URL: https://github.com/apache/pinot/pull/18486#discussion_r4502314366
##########
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:
Done in e167b86. Removed:
- the null check in `rejectUploadIfOutOfRetention`
- the `@Nullable` parameter and null branch in
`parseTableDataRetentionMillis`
- the `parseTableDataRetentionMillis(null)` assertion
- the mock-only `testRejectUploadSkippedWhenValidationConfigNull`
Missing and invalid retention on a non-null config are still covered by
`testRejectUploadSkippedWhenRetentionNotConfigured` and
`testRejectUploadSkippedWhenRetentionInvalid`.
##########
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:
Done in e167b86 for `RetentionUtilsTest`, and for the new assertions in
`ControllerConfTest` and `SegmentUploadIntegrationTest`. I left the existing
qualified `Assert.` calls in those two files as they were, to keep the diff
focused.
--
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]
