Add support for handling scenarios where end time is invalid during RetentionManager run#18148
Conversation
…etentionManager run
| return false; // Incomplete segments don't have final end time and should not be purged | ||
| } | ||
|
|
||
| return isPurgeable(tableNameWithType, segmentZKMetadata.getSegmentName(), segmentZKMetadata.getEndTimeMs()); |
There was a problem hiding this comment.
Let's add the new checks inside this method so that this method can also handle fallback.
There was a problem hiding this comment.
I intentionally kept that super simple.
It just takes in timestamps while this function handles all the complex logic.
The caller can choose to keep things simple using the other function or use this function if a logical interpretation of the ZK metadata is needed.
There was a problem hiding this comment.
I dont think that a good idea to leave to caller to understand the difference. Caller can call any method and both method should be consistent.
Passing invalid timestamp in second method behaves differently than the other method.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #18148 +/- ##
============================================
- Coverage 63.04% 63.02% -0.03%
Complexity 1617 1617
============================================
Files 3202 3202
Lines 194718 194752 +34
Branches 30047 30055 +8
============================================
- Hits 122760 122736 -24
- Misses 62233 62269 +36
- Partials 9725 9747 +22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
noob-se7en
added
configuration
xiangfu0
left a comment
xiangfu0
left a comment
There was a problem hiding this comment.
Found a few high-signal issues; see inline comments.
| boolean oldValue = _useCreationTimeFallbackForRetention; | ||
|
|
||
| // Validate that the value is a proper boolean string | ||
| if (!"true".equalsIgnoreCase(newValue) && !"false".equalsIgnoreCase(newValue)) { |
There was a problem hiding this comment.
When this cluster config key is deleted, changedConfigs will still contain it but clusterConfigs.get(...) will be null (DefaultClusterConfigChangeHandler explicitly reports deleted keys that way). This branch treats null as invalid and keeps the old value, so removing the override never reverts to the default false until restart. Because this flag gates destructive retention deletion, the current leader can keep purging segments after an operator thinks they disabled the feature. Please handle null explicitly and reset _useCreationTimeFallbackForRetention to the default.
4 participants
Summary
segmentZKMetadata.getCreationTime()instead, so segments with missing/invalid end times can still be cleaned up.controller.retentionManager.enableCreationTimeFallback(default false) — no behavior change unless explicitly opted in.Test plan
TimeRetentionStrategyTest#testCreationTimeFallback— unit tests covering: fallback disabled (existing behavior preserved), fallback enabled with valid/recent/invalid/zero creation time, valid end time takes priority over fallbackRetentionManagerTest#testCreationTimeFallbackOnChange— verifies dynamic config toggle viaonChange()RetentionManagerTest#testRetentionWithInvalidEndTimeAndCreationTimeFallback— end-to-end: segment with invalid end time is deleted when fallback is enabled and creation time exceeds retention