9aman commented on code in PR #15142:
URL: https://github.com/apache/pinot/pull/15142#discussion_r1976538207
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/retention/strategy/RetentionStrategy.java:
##########
@@ -34,4 +34,14 @@ public interface RetentionStrategy {
* @return Whether the segment should be purged
*/
boolean isPurgeable(String tableNameWithType, SegmentZKMetadata
segmentZKMetadata);
-}
+
+ /**
+ * Returns whether the segment should be purged based on end time.
+ *
+ * @param segmentName The name of the segment to check
+ * @param tableNameWithType Table name with type
+ * @param endTimeMs The end time of the segment in milliseconds
+ * @return Whether the segment should be purged
+ */
+ boolean isPurgeable(String segmentName, String tableNameWithType, long
endTimeMs);
Review Comment:
Yes, but I would still keep it as part of the `RetentionManager`. I feel
that the `SegmentDeletionManager` should just have the job of deleting (moving
to Deleted_Segments dir) and not know what segments needs to be deleted.
For our case, the RetentionManager can fetch the segments from deepstore and
decide based on the file modification time whether they need to be deleted or
not.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/retention/strategy/RetentionStrategy.java:
##########
@@ -34,4 +34,14 @@ public interface RetentionStrategy {
* @return Whether the segment should be purged
*/
boolean isPurgeable(String tableNameWithType, SegmentZKMetadata
segmentZKMetadata);
-}
+
+ /**
+ * Returns whether the segment should be purged based on end time.
+ *
+ * @param segmentName The name of the segment to check
+ * @param tableNameWithType Table name with type
+ * @param endTimeMs The end time of the segment in milliseconds
+ * @return Whether the segment should be purged
+ */
+ boolean isPurgeable(String segmentName, String tableNameWithType, long
endTimeMs);
Review Comment:
Yes, but I would still keep it as part of the `RetentionManager`. I feel
that the `SegmentDeletionManager` should just have the job of deleting (moving
to Deleted_Segments dir) and not know what segments needs to be deleted.
For our case, the `RetentionManager` can fetch the segments from deepstore
and decide based on the file modification time whether they need to be deleted
or not.
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
