shuturmurgh commented on code in PR #18486:
URL: https://github.com/apache/pinot/pull/18486#discussion_r4432677515
##########
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:
Good catch, thanks for tracing the watermark behavior. I've documented this
on the config key in `ControllerConf` and in the `rejectUploadIfOutOfRetention`
Javadoc (75d3a2c). The docs now say:
- Purge, UpsertCompaction, and RefreshSegment always upload through POST
`/segments`, so they are always subject to the check.
- RealtimeToOffline, MergeRollup, and UpsertCompactMerge are subject to it
unless batchSegmentUpload is enabled.
- A rejected RealtimeToOffline upload doesn't advance the watermark, so the
same window is retried until the source segments age out. With the hybrid
retention strategy that can go on indefinitely, so operators should check task
schedules and buffer periods against table retention before enabling the flag.
I didn't add an exemption in this PR. As you noted, the only signal today is
the SEGMENT_ZK_METADATA_CUSTOM_MAP_MODIFIER header, which is too indirect. A
clean exemption needs a dedicated marker header from minion, and that change
has mixed-version implications. I'd rather do it as a follow-up. Happy to open
an issue for it if that works for you.
##########
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:
Good catch, thanks for tracing the watermark behavior. I've documented this
on the config key in `ControllerConf` and in the `rejectUploadIfOutOfRetention`
Javadoc (75d3a2c). The docs now say:
- Purge, UpsertCompaction, and RefreshSegment always upload through POST
`/segments`, so they are always subject to the check.
- RealtimeToOffline, MergeRollup, and UpsertCompactMerge are subject to it
unless batchSegmentUpload is enabled.
- A rejected RealtimeToOffline upload doesn't advance the watermark, so the
same window is retried until the source segments age out. With the hybrid
retention strategy that can go on indefinitely, so operators should check task
schedules and buffer periods against table retention before enabling the flag.
I didn't add an exemption in this PR. As you noted, the only signal today is
the `SEGMENT_ZK_METADATA_CUSTOM_MAP_MODIFIER` header, which is too indirect. A
clean exemption needs a dedicated marker header from minion, and that change
has mixed-version implications. I'd rather do it as a follow-up. Happy to open
an issue for it if that works for you.
--
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]
