xiangfu0 commented on code in PR #18486:
URL: https://github.com/apache/pinot/pull/18486#discussion_r4432196900
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/upload/SegmentValidationUtils.java:
##########
@@ -130,6 +135,51 @@ private static String
getPartitionColumn(InstanceAssignmentConfig instanceAssign
return replicaGroupPartitionConfig != null ?
replicaGroupPartitionConfig.getPartitionColumn() : 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, using the same boundary as the controller
retention manager. **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.
+ ///
+ /// 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) {
+ return;
+ }
+ if (!RetentionUtils.shouldManageTimeBasedDataRetention(tableConfig)) {
+ return;
+ }
+ OptionalLong retentionMsOpt =
RetentionUtils.parseTableDataRetentionMillis(validationConfig);
+ if (retentionMsOpt.isEmpty()) {
+ return;
+ }
+ long retentionMs = retentionMsOpt.getAsLong();
+ if (RetentionUtils.isPurgeable(tableConfig.getTableName(),
segmentMetadata, retentionMs, currentTimeMs, false)) {
Review Comment:
This also applies to minion-produced uploads.
`BaseMultipleSegmentsConversionExecutor` pushes to `POST /segments` (unless
`batchSegmentUpload` is on, which goes to `/segments/batchUpload` and skips
this check). For RealtimeToOffline, `postProcess` only advances the watermark
after a successful upload, so a 403 here makes the generator retry the same
window on every run. That continues until the source realtime segments age out,
and indefinitely with the hybrid retention strategy, because the time boundary
can't advance.
Could we either exempt task-originated uploads or at least document this
interaction on the config key so operators know before enabling it? Today the
only signal is the `SEGMENT_ZK_METADATA_CUSTOM_MAP_MODIFIER` header that minion
sends, so a cleaner exemption would need a dedicated marker.
##########
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(
Review Comment:
This duplicates the unit/value parsing in
`TableConfigRetentionUtils.buildRetentionStrategy`, and the two already drifted
once (the `Locale.ROOT` fix). Since this now lives in `pinot-common`, could
`buildRetentionStrategy` call `parseTableDataRetentionMillis`? Then the upload
check and the purge path can't disagree about what the table's retention is.
--
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]
