jadami10 commented on code in PR #13879:
URL: https://github.com/apache/pinot/pull/13879#discussion_r1731646625
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -1505,7 +1505,7 @@ public void uploadToDeepStoreIfMissing(TableConfig
tableConfig, List<SegmentZKMe
long retentionMs =
TimeUnit.valueOf(validationConfig.getRetentionTimeUnit().toUpperCase())
.toMillis(Long.parseLong(validationConfig.getRetentionTimeValue()));
RetentionStrategy retentionStrategy = new
TimeRetentionStrategy(TimeUnit.MILLISECONDS,
- retentionMs -
MIN_TIME_BEFORE_SEGMENT_EXPIRATION_FOR_FIXING_DEEP_STORE_COPY_MILLIS);
+ retentionMs -
MIN_TIME_BEFORE_SEGMENT_EXPIRATION_FOR_FIXING_DEEP_STORE_COPY_MILLIS,
_controllerMetrics);
Review Comment:
not exactly. We'd have to duplicate the logic in
https://github.com/apache/pinot/blob/master/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/retention/RetentionManager.java#L95
into the status checker as well. Or make "isRetentionEnabled" a static
function and call it.
Either way, seems like we disagree on where to place this. Let's get a third
opinion. cc @Jackie-Jiang, curious on your thoughts since you've most recently
touched both the `SegmentStatusChecker` and `TimeRetentionStrategy` code
--
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]
