Skip missing consuming segment check for paused realtime tables - #19310
Open
rohityadav1993 wants to merge 1 commit into
Open
Skip missing consuming segment check for paused realtime tables#19310rohityadav1993 wants to merge 1 commit into
rohityadav1993 wants to merge 1 commit into
Conversation
SegmentStatusChecker invoked MissingConsumingSegmentFinder for every enabled REALTIME table without checking whether ingestion was paused. Pausing deliberately leaves a partition with no CONSUMING segment once the current one commits, so the finder read that as a missing consumer and the missingConsumingSegment* gauges reported a growing false positive for as long as the pause lasted. Gate the finder on the table pause state already computed in updateSegmentMetrics. Because these gauges are last-write-wins with no "unset" value, the paused branch actively resets them via a new MissingConsumingSegmentFinder.resetMetrics, keeping ownership of the gauge names with the class that emits them.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19310 +/- ##
============================================
- Coverage 67.20% 67.18% -0.03%
Complexity 1424 1424
============================================
Files 3462 3462
Lines 220361 220368 +7
Branches 35147 35148 +1
============================================
- Hits 148087 148046 -41
- Misses 60467 60518 +51
+ Partials 11807 11804 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
rohityadav1993
marked this pull request as ready for review
August 19, 2026 06:54
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
bugmetricSummary
missingConsumingSegmentTotalCountand its two sibling gauges report a false positive for REALTIME tables whose ingestion is intentionally paused. This gates the check on the table's pause state and resets the gauges while paused.Problem
SegmentStatusChecker.updateSegmentMetricsinvokesMissingConsumingSegmentFinderfor every enabled REALTIME table, with no check for whether the table is paused via the pause/resume ingestion API.When a table is paused,
PinotLLCRealtimeSegmentManagerdeliberately does not create a replacement CONSUMING segment after the current one commits, leaving the partition with only a completed segment.MissingConsumingSegmentFindertreats exactly that shape — no consuming segment, a completed segment, and a stream offset that has advanced past it — as a missing consumer. Since the stream keeps advancing while paused, the condition never clears, soMISSING_CONSUMING_SEGMENT_MAX_DURATION_MINUTESgrows for the duration of the pause. The result is indistinguishable from a genuine ingestion failure.Disabled tables are unaffected:
updateSegmentMetricsreturns early on!idealState.isEnabled()and callsremoveMetricsForTable. Paused tables had no equivalent guard.The existing per-topic
PauseState.getIndexOfInactiveTopics()exclusion inside the finder does not cover this.updatePauseStateInIdealStateonly carries forward a pre-existing inactive-topic list; a whole-table pause never populates it, soisPaused() == trueleaves the finder's per-topic exclusion empty.Fix
Reuse the pause state already computed in
updateSegmentMetricsfor theTABLE_CONSUMPTION_PAUSEDgauge, and skip the finder when the table is paused.These gauges are last-write-wins with no "unset" value, so simply skipping the call would leave whatever value was written before the pause in place, alerting indefinitely. The paused branch therefore actively resets them through a new
MissingConsumingSegmentFinder.resetMetrics, which keeps the set of gauge names in the class that emits them — otherwise a future gauge added tofindAndEmitMetricswould silently go stale on the paused path, reintroducing this same bug.No config, REST, metric-name, or wire-format changes. The only new surface is
MissingConsumingSegmentFinder.resetMetrics, an internal helper on an existing controller-side class.Known gap / possible follow-up
This gates on whole-table pause (
isTablePaused) only. Individually paused topics in a multi-topic stream config (PauseState.getIndexOfInactiveTopics(), added in #16692) are already handled for the ordinary case:PartitionGroupMetadataFetcher.fetchMultipleStreamsskips paused topic indices, so those partitions never enter_partitionGroupIdToLargestStreamOffsetMapand the main detection loop never examines them.There is a narrower gap, which I have left alone here. When that map ends up empty,
findMissingSegmentsfalls back to iteratingpartitionGroupIdToLatestCompletedSegmentMapand counts every partition without a consuming segment as missing, consulting neither pause state nor stream offsets. Pausing every topic of a multi-topic table individually reaches that fallback. The same fallback is also taken when the stream-metadata fetch throws, which is existing behaviour and a separate question.That case seemed better handled on its own, since it is about the fallback path's semantics rather than the pause gate this PR adds. Happy to follow up separately, or to fold it in here if reviewers prefer.
Test Plan
New
SegmentStatusCheckerTest.realtimePausedTableHasNoMissingConsumingSegmentAlert, built on the existingrealtimeBasicTestfixture and run in two phases against the same metrics instance:MISSING_CONSUMING_SEGMENT_TOTAL_COUNT == 2, establishing a non-zero readingMISSING_CONSUMING_SEGMENT_*gauges are0The two-phase shape is deliberate: it covers the actual production scenario, where the gauge is already reporting non-zero from an earlier run and the pause must actively clear it, rather than merely never setting it.
Verified the test fails without the fix. With
SegmentStatusChecker.javareverted to master, the run fails on the second phase withexpected [0] but found [2], and the surefire log contains noCaught exception while updating segment status— confirming the failure is a real computed value rather than a swallowed exception leaving the gauge at its default.SegmentStatusCheckerTest: 42/42 passMissingConsumingSegmentFinderTest: 6/6 pass, no regressionspotless:apply,checkstyle:check,license:checkonpinot-controller: clean