Advance consume offset past read_committed control-record tails - #19267
Open
Vamsi-klu wants to merge 1 commit into
Open
Advance consume offset past read_committed control-record tails#19267Vamsi-klu wants to merge 1 commit into
Vamsi-klu wants to merge 1 commit into
Conversation
Kafka already returns the real next offset after an empty committed poll, but the consume loop discarded it when no user records were present, so freshness readiness never caught up on idle EOS tables. Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19267 +/- ##
============================================
- Coverage 66.97% 66.96% -0.02%
Complexity 1423 1423
============================================
Files 3453 3453
Lines 218949 218961 +12
Branches 34805 34806 +1
============================================
- Hits 146648 146622 -26
- Misses 60585 60624 +39
+ Partials 11716 11715 -1
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:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes #17962 by advancing realtime consumption beyond invisible Kafka transactional control-record tails.
Changes:
- Advances
_currentOffsetfor authoritative empty-batch offsets. - Documents the
MessageBatchoffset contract. - Adds unit, Kafka, readiness, and integration coverage.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
pinot-spi/.../MessageBatch.java |
Clarifies next-batch offset semantics. |
pinot-core/.../RealtimeSegmentDataManager.java |
Advances offsets for empty control-record tails. |
pinot-core/.../RealtimeSegmentDataManagerTest.java |
Tests advanced and unchanged empty offsets. |
pinot-server/.../FreshnessBasedConsumptionStatusCheckerTest.java |
Prevents unsafe latest - 1 readiness logic. |
pinot-kafka-3.0/.../KafkaPartitionLevelConsumerTest.java |
Verifies Kafka 3 position snapping. |
pinot-kafka-4.0/.../KafkaPartitionLevelConsumerTest.java |
Verifies Kafka 4 position snapping. |
pinot-integration-tests/.../ExactlyOnceKafkaRealtimeClusterIntegrationTest.java |
Validates end-to-end control-tail catch-up. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
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.
Problem
Under Kafka
isolation.level=read_committed, a partition whose log tail is only transaction control records never advances_currentOffset.FreshnessBasedConsumptionStatusCheckerthen waits forever on a stable one-offset gap, and the server never reports GOOD. Rolling restarts of low-volume exactly-once tables loop.The reporter case: last visible record at 18895, Pinot sitting at 18896, Kafka latest at 18897 (the commit marker).
isOffsetCaughtUprequirescurrent >= latest, so the partition never catches up.KafkaPartitionLevelConsumeralready has the right answer. On an emptyread_committedpoll it snaps_nextReadOffsettoKafkaConsumer.position()and returns that asoffsetOfNextBatch.RealtimeSegmentDataManagerthrows it away because the advance is gated ongetUnfilteredMessageCount() > 0, and a control-record-only tail yields zero records.What I did
I widened that one guard so the consume loop also advances when the next-batch offset is already ahead of the current offset. I also documented the
MessageBatch.getOffsetOfNextBatch()contract: return the requested start offset unchanged when the partition had nothing to hand back; only return a larger offset when the stream itself has moved past offsets this batch will never deliver.How I did it
In
RealtimeSegmentDataManager:getOffsetOfNextBatch()is generic SPI. Every other stream returns the start offset unchanged on a genuinely empty batch, so Kinesis and Pulsar behavior does not change. A partition that is genuinely lagging also returns the unchanged start offset and stays not-caught-up.This is not a
latest - 1heuristic. I did not changeDEFAULT_REALTIME_FRESHNESS_IDLE_TIMEOUT_MS,endOffsetsforread_uncommitted, orKafkaStreamMetadataProvider.fetchLatestStreamOffset.Impact
Idle EOS tables under
read_committedcan finish catch-up after a restart. Servers that hung inRealtimeConsumptionCatchupServiceStatusCallbackcan turn GOOD. Genuinely lagging partitions,read_uncommitted, Kinesis, and Pulsar are unchanged.Testing
RealtimeSegmentDataManagerTest#testEmptyBatchWithAdvancedNextOffsetMovesCurrentOffsetfails before this change (stays at the old offset) and passes after. The siblingtestEmptyBatchWithUnchangedNextOffsetDoesNotInventOffsetasserts an empty batch whose next offset did not move invents no offset.KafkaPartitionLevelConsumerTest#testReadCommittedEmptyPollSnapsOffsetOfNextBatchToPosition(kafka 3.0 and 4.0) pins theposition()snap the guard depends on.ExactlyOnceKafkaRealtimeClusterIntegrationTestnow waits for consuming offsets to reach latest after the final commit marker.FreshnessBasedConsumptionStatusCheckerTest#controlRecordTailDoesNotTreatLatestMinusOneAsCaughtUpis a behavior lock, not a regression test: it passes either way and exists so nobody "fixes" readiness with alatest - 1heuristic.Fixes #17962
Made with Cursor