Repository navigation
fix: runlength decoding after no-progress commit - #563
Merged
John Byrne (johnbyrnejb) merged 1 commit intoMar 8, 2023
Merged
John Byrne (johnbyrnejb) merged 1 commit into
John Byrne (johnbyrnejb) merged 1 commit into
Conversation
Nacho Muñoz Gómez (nachomdo)
approved these changes
Mar 6, 2023
Nacho Muñoz Gómez (nachomdo)
left a comment
Nacho Muñoz Gómez (nachomdo)
left a comment
Contributor
There was a problem hiding this comment.
LGTM 👍
John Byrne (johnbyrnejb)
approved these changes
Mar 8, 2023
6 tasks done
Antony Stubbs (astubbs)
added a commit
to astubbs/parallel-consumer
that referenced
this pull request
Sep 8, 2026
…d the invariant that keeps it shut is pinned (#484) WHAT WAS OPEN. confluentinc#546 holds three defects behind one warning string. One is fixed by inheritance (confluentinc#563, already in this tree and guarded in OffsetRunLength). One is live. The third - the "Bootstrap polled offset has been reset to an earlier offset" branch of PartitionState#maybeTruncateBelowOrAbove - was never diagnosed upstream, and this fork's note carried an UNTESTED hypothesis for it, proposing a control-arm test nobody had run. This change runs it, and the hypothesis is refuted. THE HYPOTHESIS, AND WHY IT NAMES THE WRONG BRANCH. The note proposed a race: PC issues its own consumer.committed() in loadPartitionStateForAssignment, separate from the consumer's own position resolution for a newly assigned partition, so a commit landing between the two reads would leave PC one commit ahead of where the fetcher starts. It is not a race at all. In kafka-clients 3.9.2 - the version this tree builds on - ClassicKafkaConsumer#updateAssignmentMetadataIfNeeded runs coordinator.poll, which invokes the rebalance listener and therefore PC's committed(), and only afterwards updateFetchPositions, whose ConsumerCoordinator#initWithCommittedOffsetsIfNeeded is what gives an initializing partition its position. PC's read strictly PRECEDES the fetcher's, so a commit landing between the two is the one the FETCHER sees: the polled offset comes out at or above PC's expectation. The proposed mechanism can only produce the other warning. WHAT IS LEFT, AND THE INVARIANT THAT SHUTS IT. One route into the branch belongs to PC rather than to the broker: a commit whose encoded payload decodes to an expectation ABOVE the offset that commit was filed under - the shape #337 closed. PartitionStateBootstrapTruncation162Test round-trips a commit PC itself wrote back through MockConsumer, WorkManager#onPartitionsAssigned and the decoders, over four shapes PC can genuinely commit (fully drained, contiguous tail incomplete, a completed island above the commit frontier, only the base offset incomplete), and asserts the bootstrap expectation equals the committed offset exactly. Hold that invariant and a partition PC hands to itself cannot take either truncation branch. Every remaining way in is the broker handing back a position below the committed offset - an offset reset, a manual rewind, another member committing lower, a stale OffsetFetch across a coordinator failover - and there replaying is the right answer, not a defect. THE TWO BRANCHES AS A CONTROLLED PAIR. One fixture, one differing term - the offset the first poll batch starts at - so the outcome that flips is the branch's own. Below expected: every loaded incomplete is discarded, including one above the batch that the batch does not re-register, and offsetHighestSucceeded resets, so records the previous owner completed and committed are processed again. Above expected, the control: only incompletes beneath the polled offset are pruned, and the one above the batch survives. The note's "duplicate processing, not loss" claim is asserted in BOTH halves - nothing tracked can be stranded below the polled offset, because the expectation the branch fires against IS the lowest tracked incomplete. NEGATIVE CONTROLS, ONE PER ASSERTION, so no arm can pass by accident. Removing the reset fails only the below-expected arm; removing the prune fails only the above-expected arm; dropping the payload's lowest incomplete on decode flips the two round-trip shapes that carry a payload and leaves untouched the two that do not. AN INSTRUMENT THAT LIED FIRST, recorded because it read as a product defect. The round trip initially reported an expectation of 0 for three of the four shapes. MockConsumer#committed hands back a bare OffsetAndMetadata(0) - indistinguishable from a real commit at offset 0 - for any partition its own SubscriptionState does not hold, and MockConsumer#assign clears the committed map, so the assignment has to precede the commit. The arm now proves itself, and the comment at that line says why. A test harness that fabricates a plausible wrong answer is worth writing down: it cost a diagnosis before it was caught. WHAT REMAINS OPEN ON #162, and why this does not close it. The false warning: maybeTruncateBelowOrAbove never asks whether commit data existed, so a partition with no committed offset reports "expected 0 from loaded commit data" and truncates nothing. Misdirection, still untested, and unchanged here. The inflight note is shrunk to that one case and the draft mirror reply is corrected to name it, so the upstream thread is not told that both remaining cases are covered. src/docs/development/upstream-map.yaml records the lifecycle transition in the commit that caused it. CROSS-CHECKED AGAINST THE MASTER THIS LANDS ON. #470 rewrote PartitionState's commit-acknowledgement path while this branch was open, and its offerLastMadeForCommit javadoc names this same downward reset from the other side - only the latest commit offer is kept rather than a set, because the bootstrap reset is "exactly the event no pruning rule would see". The two records agree: the reset is real, reachable only in bootstrapPhase, and destructive of anything keyed to the pre-reset expectation. Nothing in the refutation is disturbed, because the refutation is about how the branch is ENTERED, not what it does once entered. Re-run after merging master rather than assumed. No production code changes: one new test class, the inflight note, and the upstream manifest. Upstream-Issue: confluentinc#546 Co-authored-by: Claude Opus (1M context) <noreply@anthropic.com>
3 of 6 tasks
Antony Stubbs (astubbs)
added a commit
to astubbs/parallel-consumer
that referenced
this pull request
Sep 9, 2026
…bsence is recorded rather than inferred (#494) WHAT WAS WRONG. PartitionState#maybeTruncateBelowOrAbove took its bootstrap expectation from getOffsetToCommit() and never asked whether commit data existed. A partition assigned with no committed offset is built from OffsetMapCodecManager.HighestOffsetAndIncompletes.of(), so offsetHighestSucceeded is KAFKA_OFFSET_ABSENCE and the expectation computes to 0 - and any first poll above offset 0 took the above-expected branch, logged "Truncating state - removing records lower than ..." at WARN, and pruned nothing, because there was nothing loaded to prune. False in both halves. It fired for the normal state of a new consumer group, and for a group whose committed offset had aged out of the offsets topic. That is the shape reported upstream on 0.5.2.7 - after confluentinc#563 shipped and fixed the OTHER form of the same string - alongside a broker CLI screenshot showing an empty CURRENT-OFFSET for exactly the partitions that warned. THE BEHAVIOUR NOW. Absent commit data reports at INFO and takes no truncation branch: "No committed offset for partition N of topic T - starting from the first polled offset X. Nothing was loaded, so nothing is being removed. Expected for a new consumer group, or where the group's committed offset has aged out of the offsets topic." The WARN is unchanged for the genuine cases - a committed offset the broker no longer has records for, compaction, a raised committed offset - so an operator alerting on "Truncating state" keeps exactly the alert they had, minus the false firings. WHY NOT THE SENTINEL, WHICH IS THE WHOLE OF THE DESIGN. The cheap check reads offsetHighestSucceeded == KAFKA_OFFSET_ABSENCE, and it is WRONG: a real commit filed at offset 0 with no offset map decodes to a highest-seen of 0 - 1, which is that same sentinel, with commit data that genuinely existed and an expectation of 0 that is genuinely meant. Testing the sentinel would silence the true truncation report for exactly the partition most likely to be caught by retention - the one that has processed nothing yet. -1 means "nothing succeeded", not "no commit arrived". So the absence is carried from where it is known: every decode path yields a PRESENT HighestOffsetAndIncompletes#getHighestSeenOffset, because every decode has a committed offset to be relative to, and only the default entry yields an empty one. PartitionState samples that into a final boolean in its constructor, before initStateFromOffsetData collapses the Optional into the sentinel - the engine AGENTS.md collapse-parallel-state rule applied to a fact rather than a cache. The field being final is also the whole thread-safety story: publication is the constructor's freeze, and it is read on the control thread in the same statement that closes bootstrapPhase. RED FIRST, THEN THE CONTROL ARM. PartitionStateAbsentCommitData162Test was red on master in three arms - the first poll above 0 warned, the assignment path through a consumer reporting no committed offset warned, and the at-offset-0 arm had no INFO line to find - and green after. The fourth arm is the control: same sentinel, same expectation of 0, same first polled offset, one term differing (commit data existed), and it must still warn. Prediction stated before running: substituting the sentinel shorthand for the recorded fact fails ONLY that arm. Run with the shorthand substituted, it did exactly that - one failure, the control arm, with the absent-data arms and PartitionStateBootstrapTruncation162Test's two genuine branches still green. A CLAIM CORRECTED ON THE WAY PAST. The inflight note said #217's foreign-metadata recovery lands on the same default entry through catch (OffsetDecodingError), and so inherits the false warning. It does not. Under the runtime-default IGNORE policy, EncodedOffsetPair#handleUnreadableMetadata returns of(baseOffset - 1) - commit data carrying the committed offset - so such a partition bootstraps with a real expectation and the truncation branches stay correct for it; under FAIL the typed exception propagates instead. Neither reaches the default entry, and the only production caller of OffsetEncoding#decode is EncodedOffsetPair#unwrap, which has test callers only. Asserted by the test's last arm; the now-dead catch is recorded in docs/refactoring.md. DEFECT-CLASS SWEEP - other places deriving "no commit data" from sentinel arithmetic. Every reader of KAFKA_OFFSET_ABSENCE and of the offset accessors in main was checked, and none is the same defect: OffsetSimultaneousEncoder's highestSucceededOffset == KAFKA_OFFSET_ABSENCE asks about SUCCESS, which the sentinel answers faithfully; isRecordPreviouslyCompleted's recOffset <= offsetHighestSucceeded is correct under both readings; RemovedPartitionState returns the sentinel as a deliberate no-op contract and never reaches truncation; RunLengthEncoder's use is a loop initialiser. Two sites borrow the constant for a second meaning without being defects, and are recorded in docs/refactoring.md: getOffsetHighestSequentialSucceeded uses it as a below-every-offset floor for ceiling(), and PartitionStateManager#incrementPartitionAssignmentEpoch defaults an EPOCH from an OFFSET constant. WHY THIS CLOSES THE ISSUE. All three defects behind confluentinc#546's one warning string are settled here: the "expected 1" RunLength decode by inheritance from confluentinc#563; the "expected 0 from loaded commit data" form by this change; and the "reset to an earlier offset" branch refuted as a PC-reachable defect by #484. Upstream's own issue stays open - that repository is no longer maintained. RECORDS. docs/inflight/bug-162-offset-state-truncation.md is deleted, which is its own "Delete when" condition. What outlives it - the three-defects reading, the sentinel-versus-fact discriminator and the #217 correction - is migrated to docs/solutions/logic-errors/. The drafted mirror reply moves to docs/inflight/issue-response-162.md, held for the pre-release sweep rather than deleted with this PR. Citations in process-candidate-ranking.md and upstream-mirror-bodies-are-stale.md are repaired, and src/docs/development/upstream-map.yaml records the lifecycle transition. Closes #162 Upstream-Issue: confluentinc#546 A SIBLING FIELD, AND WHY IT IS NOT THE SAME FACT. #480 landed while this was open and added committedOffsetTheMapWasLoadedAgainst to the same constructor, defaulting to the same absence sentinel, so the next reader meets two fields that look like one and deletes one. They differ: that field records what the caller passed, and NO_OFFSET_MAP_WAS_LOADED is also what every direct constructor call passes; this one is derived from the offset data itself, so it answers what the codec produced. Measured, not argued - substituting one for the other fails six assertions across three test classes, because partitions that did load commit data would go quiet. The javadoc records that and names what would collapse the two into one: the constructor gaining the committed offset, at which point this field is deleted rather than kept in sync. #480's refusal path, which re-initialises from the offset data a third time, deliberately does not re-sample this field either: a refused map still leaves a real committed offset to bootstrap against. Co-authored-by: Claude Opus (1M context) <noreply@anthropic.com>
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
3 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.
When offset data is serialized using RunLength encoder - and base offset to commit is same as the next expected offset - 1 (or lower) - basically there is no extended metadata to encode - could be caused by slow processing (slower than consumption) or on rebalance etc.
When that data is subsequently decoded - the HighestOffset is returned as 0 - that causes the warning about state truncation to be shown and the bootstrap state truncation logic to be invoked.
This fix addresses the decoding side - initializing the HighestOffset = baseOffset-1 - same as bitset decoder, but potentially we could detect and skip encoding of metadata in those scenarios - need to dig deeper and can be done as follow up.
Related issue #546
Checklist