shuturmurgh commented on code in PR #18486:
URL: https://github.com/apache/pinot/pull/18486#discussion_r4502307259


##########
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:
   Done in e167b86. The key is now 
`controller.segment.upload.reject.out.of.retention`, still defaulting to 
`false`.
   
   - Constants: `SEGMENT_UPLOAD_REJECT_OUT_OF_RETENTION` and 
`DEFAULT_SEGMENT_UPLOAD_REJECT_OUT_OF_RETENTION`
   - Accessors: `isSegmentUploadRejectOutOfRetention()` and 
`setSegmentUploadRejectOutOfRetention(boolean)`
   - Also updated: the `SegmentValidationUtils` Javadoc, 
`SegmentUploadIntegrationTest` and the PR description
   
   No compatibility alias, since the key hasn't been released.



##########
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:
   Added



-- 
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]

Reply via email to