Jackie-Jiang commented on code in PR #19415:
URL: https://github.com/apache/pinot/pull/19415#discussion_r3907449520
##########
pinot-common/src/main/java/org/apache/pinot/common/tier/TimeBasedTierSegmentSelector.java:
##########
@@ -43,20 +79,39 @@ public boolean selectSegment(String tableNameWithType,
SegmentZKMetadata segment
return false;
}
- // get segment end time to decide if segment gets selected
- long endTimeMs = segmentZKMetadata.getEndTimeMs();
- Preconditions.checkState(endTimeMs > 0, "Invalid endTimeMs: %s for
segment: %s of table: %s", endTimeMs,
- segmentZKMetadata.getSegmentName(), tableNameWithType);
- return (System.currentTimeMillis() - endTimeMs) > _segmentAgeMillis;
+ long referenceMs;
+ switch (_ageField) {
+ case CREATION_TIME:
+ referenceMs = segmentZKMetadata.getCreationTime();
+ // creationTime may be absent for very old segments predating the
field; skip rather than throw
+ // so a partially-populated table doesn't fail every tier evaluation.
+ if (referenceMs <= 0) {
+ return false;
+ }
+ break;
+ case END_TIME:
+ default:
Review Comment:
(minor) Throw exception for `default` for robustness
##########
pinot-common/src/main/java/org/apache/pinot/common/tier/TimeBasedTierSegmentSelector.java:
##########
@@ -19,16 +19,52 @@
package org.apache.pinot.common.tier;
import com.google.common.base.Preconditions;
+import javax.annotation.Nullable;
import org.apache.pinot.common.metadata.segment.SegmentZKMetadata;
import org.apache.pinot.spi.utils.TimeUtils;
-/// A [TierSegmentSelector] strategy which selects segments for a tier based
on the age of the segment
+/// A [TierSegmentSelector] strategy which selects segments for a tier based
on the age of the segment.
+///
+/// The age reference is controlled by the tier's `segmentAgeField`:
+/// - `endTime` (default, backward-compatible): uses
`SegmentZKMetadata#getEndTimeMs()`, the segment's
+/// max data timestamp. Suitable when segment age tracks data recency,
e.g. streaming ingest where
+/// endTime is close to wall-clock now.
+/// - `creationTime`: uses `SegmentZKMetadata#getCreationTime()`, when the
segment file was built.
+/// Suitable when segment age should track ingestion recency, e.g. batch
ingest of historical data
+/// where endTime lies far in the past regardless of when the segment was
created.
public class TimeBasedTierSegmentSelector implements TierSegmentSelector {
+
+ /// Which timestamp field on [SegmentZKMetadata] is compared against the age
threshold.
+ public enum AgeField {
+ END_TIME, CREATION_TIME;
+
+ public static AgeField fromConfig(@Nullable String value) {
+ if (value == null || value.isEmpty()) {
Review Comment:
(minor) StringUtils.isEmpty()
##########
pinot-common/src/main/java/org/apache/pinot/common/tier/TimeBasedTierSegmentSelector.java:
##########
@@ -43,20 +79,39 @@ public boolean selectSegment(String tableNameWithType,
SegmentZKMetadata segment
return false;
}
- // get segment end time to decide if segment gets selected
- long endTimeMs = segmentZKMetadata.getEndTimeMs();
- Preconditions.checkState(endTimeMs > 0, "Invalid endTimeMs: %s for
segment: %s of table: %s", endTimeMs,
- segmentZKMetadata.getSegmentName(), tableNameWithType);
- return (System.currentTimeMillis() - endTimeMs) > _segmentAgeMillis;
+ long referenceMs;
+ switch (_ageField) {
+ case CREATION_TIME:
+ referenceMs = segmentZKMetadata.getCreationTime();
+ // creationTime may be absent for very old segments predating the
field; skip rather than throw
Review Comment:
(nit) Capitalize the comments
##########
pinot-common/src/main/java/org/apache/pinot/common/tier/TimeBasedTierSegmentSelector.java:
##########
@@ -43,20 +79,39 @@ public boolean selectSegment(String tableNameWithType,
SegmentZKMetadata segment
return false;
}
- // get segment end time to decide if segment gets selected
- long endTimeMs = segmentZKMetadata.getEndTimeMs();
- Preconditions.checkState(endTimeMs > 0, "Invalid endTimeMs: %s for
segment: %s of table: %s", endTimeMs,
- segmentZKMetadata.getSegmentName(), tableNameWithType);
- return (System.currentTimeMillis() - endTimeMs) > _segmentAgeMillis;
+ long referenceMs;
+ switch (_ageField) {
+ case CREATION_TIME:
+ referenceMs = segmentZKMetadata.getCreationTime();
+ // creationTime may be absent for very old segments predating the
field; skip rather than throw
+ // so a partially-populated table doesn't fail every tier evaluation.
+ if (referenceMs <= 0) {
+ return false;
Review Comment:
Should we return true instead given this is very old behavior
--
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]
