Continue Freshness check if latest stream offset fetch fails #17563
Conversation
There was a problem hiding this comment.
Pull request overview
This PR restores the previous behavior where freshness checking continues even when fetching the latest stream offset fails. Before PR #17089, a null latest stream offset would not cause the server to enter a bad state. This change catches exceptions during offset fetching and allows freshness checks to proceed using other criteria (like idle timeout).
Changes:
- Added exception handling when fetching latest stream offset to prevent failures from blocking freshness checks
- Added
@Nullableannotations toisOffsetCaughtUpparameters to explicitly handle null offsets - Added comprehensive test coverage for TimeoutException scenario during offset fetching
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| FreshnessBasedConsumptionStatusChecker.java | Wraps offset fetching in try-catch to handle failures gracefully and continue with other freshness checks |
| IngestionBasedConsumptionStatusChecker.java | Adds @Nullable annotations to offset parameters to indicate they can be null |
| FreshnessBasedConsumptionStatusCheckerTest.java | Adds test verifying behavior when TimeoutException occurs during offset fetching |
| realtimeTableDataManager.getStreamMetadataProvider(rtSegmentDataManager); | ||
| latestStreamOffset = | ||
| RealtimeSegmentMetadataUtils.fetchLatestStreamOffset(rtSegmentDataManager, streamMetadataProvider); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Catching generic Exception is too broad. Consider catching specific exceptions like TimeoutException that are expected, and let unexpected exceptions propagate. This prevents masking serious errors while still handling known failure cases.
| } catch (Exception e) { | |
| } catch (RuntimeException e) { |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #17563 +/- ##
============================================
+ Coverage 63.18% 63.21% +0.02%
Complexity 1477 1477
============================================
Files 3172 3172
Lines 189773 189777 +4
Branches 29041 29041
============================================
+ Hits 119913 119964 +51
+ Misses 60547 60497 -50
- Partials 9313 9316 +3
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:
|
4 participants
Problem
Before this PR: #17089, When latest stream offset is null, Freshness checker will skip and not cause server to run in bad state. The PR caused a behaviour change.
Solution
Maintain the behaviour previous of PR: #17089 (Allow freshness checker to run if latestStream offset fetch fails.)