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


##########
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:
   Agreed. `TableConfigRetentionUtils.buildRetentionStrategy` now delegates to 
`RetentionUtils.parseTableDataRetentionMillis`, so the upload check and the 
purge path share one parser (75d3a2c). The resulting boundary is the same as 
before, and the existing `RetentionManagerTest` cases pass unchanged. I also 
added `TableConfigRetentionUtilsTest`. It covers the configured window, 
lowercase units, invalid or missing retention, and propagation of the 
creation-time fallback flag.
   
   While doing this I also dropped the helper's parse-failure log to DEBUG, so 
a misconfigured table logs one warning per retention cycle instead of two.



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