Repository navigation
fix(logging) astubbs#168: bound the commit-error line, keep every offset - #428
Conversation
The ERROR line PC logs when an asynchronous offset commit fails
("Error committing offsets: ...") interpolated the whole
Map<TopicPartition, OffsetAndMetadata>. Each entry's metadata is PC's
base64-encoded offset map - up to OffsetMapCodecManager.DefaultMaxMetadataSize
(4096) characters per partition - so the line grew to partitions x 4KB and log
tooling truncated it on precisely the occasion an operator needed it whole.
This is the third instance of the class #203 fixed the first two of.
For a user: the line now reads
Error committing offsets: 2 partitions: orders-0: offset 1000, 812 chars of
metadata; orders-1: offset 5, no metadata, exception: ...
naming EVERY partition and its offset - which is what #168
(confluentinc#629) asked for, and why no partition cap is applied here even
though the batch renderings in the same class carry one - and reducing only the
metadata to its length, which is itself the diagnostic when a commit is rejected
for its size. The unabridged map is logged at DEBUG immediately after
("Failed commit in full: ..."), where it has to be asked for.
Rendering lives in the shared RecordBatchSummary (new summariseCommit), not a
second format string; its comparator is generalised so both renderings sort the
same way. The callback's behaviour is unchanged - the TODO about marking async
commits successful before the broker answers is a separate recorded bug
(docs/inflight/bug-async-commit-marked-successful-before-broker-ack.md).
The tests read the emitted line through LogCapture rather than the format
string, so re-interpolating the map fails a test. Two hazards that only bite
under the module's concurrent execution were found while making them pass, and
both are recorded at the annotations: two captures of the same logger race on
LogCapture's level restore, so the capturing methods take SAME_THREAD (the
precedent LoadFactorCeilingReportingTest already carries); and
OffsetMapCodecManager.DefaultMaxMetadataSize is a mutable static that two
back-pressure tests write, so every test reading it takes the READ side of the
resource lock those two already hold for writing.
Found and not fixed here: the two "Offset commit deferred" WARN lines in the
same class - the sync-mode path for the same two exceptions - name no partition
or offset at all, contradicting what the #168 mirror body claims about
the sync path. The offsets are one frame down, and PR
#352 is adding the field that carries them, so it is
recorded in docs/inflight/bug-deferred-commit-warn-names-no-offsets.md and
handed there rather than fought over in two PRs.
Upstream-Issue: confluentinc#629
Forwarded: no
Applied-Upstream: no
… the line was wrong Findings from a local ce-simplify and ce-code-review pass over this branch. Each was reproduced by mutating the production code and watching the suite fail, then restored - the same prove-it-red discipline as the original change. The success-path test asserted only that nothing was logged at ERROR. Both log statements sit behind one `exception != null` guard, so moving or duplicating the unabridged DEBUG dump outside it would have put the whole offset map - every partition's metadata, the 4KB-per-partition string this issue is about - on the hot path of every SUCCESSFUL commit, with the entire suite still green. Three reviewers found this independently. The test now asserts that dump is absent too, and is renamed for what it actually guards. Verified: the mutation fails it. Nothing asserted the ERROR event still carried the exception. LogCapture's messagesAt() formats the message and drops the throwable, so deleting the trailing argument left every existing assertion passing while halving the line's diagnostic value. Asserted now via the events() accessor LogCapture already exposed - no new helper. The assertion names its own cause rather than dereferencing a null proxy, so the mutation reports "no throwable attached - was the exception argument dropped?" instead of an NPE inside the test. Also corrected, all reviewer-found: - RecordBatchSummary's class javadoc still promised that its helpers cap the partitions they name. summariseCommit deliberately does not, so the contract contradicted the method this change added. The class javadoc and MAX_PARTITIONS_LISTED now say which rule belongs to which rendering. - The DEBUG read filtered on the shared literal "Failed commit in full" instead of this test's own topic, which is the scoping obligation LogCapture states and the class's own comment claimed to follow. - The concurrency comments overstated the hazard. The module sets junit.jupiter.execution.parallel.mode.default=concurrent but leaves mode.classes.default at same_thread, so METHODS of one class run in parallel while classes do not. @execution(SAME_THREAD) is therefore what fixes the observed capture race; the @ResourceLock is belt-and-braces against a future change to that setting. Said plainly rather than implying both are load-bearing. - upstream-map's fork.status stayed `pr-open` while the same commit recorded that #203 had merged and this branch had no PR - so `pr-open` described nothing. It is `in-progress` until the PR exists. Upstream was actually re-checked this time (confluentinc#629 and confluentinc#640 open, #57 closed), which is what the bumped last_checked is supposed to mean. Upstream-Issue: confluentinc#629 Forwarded: no Applied-Upstream: no
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
The entry and the note could not carry this number before the PR existed. fork.status returns to pr-open now that it is true of something again. Upstream-Issue: confluentinc#629 Forwarded: no Applied-Upstream: no
[superseded - a quarantined test changed outcome] 🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its Updated for Superseded by a newer quarantine lane report. |
[superseded - a quarantined test changed outcome] 🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its No quarantined test changed outcome since the previous push. Updated for Superseded by a newer quarantine lane report. |
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
No new clones introduced by this PR. Powered by astubbs/duplicate-code-cross-check |
|
🟢 Throughput — OKThis branch measured about 13% faster than master, on the one test this measures. That is INSIDE this test's own run-to-run spread of about 17%, so read it as a reading and not as a result - re-running the same commit moves it by about as much.
Allowable range 🟢 ≥ 0.70 · 🟡 0.50–0.70 (about a 30% loss) · 🔴 < 0.50 (about a 50% loss) What the numbers mean, and what they cannot tell youThe one that gets misread. Why a shape and not a rate. A rate depends on which runner you drew. A shape does not: every test here processes a fixed number of records, so a runner twice as slow doubles the subject and the controls together and leaves their ratio alone. That is the whole trick, and it is why the reported rate is shown last and labelled as this machine only. Reading the comparison. By conservation, not by correction. Every test in this lane processes a fixed number of records, so within one run the ratio of one test's time to another's is invariant under machine speed — a runner twice as slow doubles both terms and leaves the ratio alone. There is no machine-index correction to be wrong, because nothing needed correcting. Per-method times, not class times. A class time is Reference is the median of 10 recent What this still cannot do. It removes machine-to-machine variance. It does not remove this test's own run-to-run variance, measured at about 30% on a single unchanged commit while its controls stayed within 5%. That is a property of the test, not of the comparison, and no arithmetic here can touch it — which is why the reference is a median and the bounds are deliberately coarse. 🟡 means look at this; only 🔴 is outside the measured spread. Runs used: 440b9bd, 11e0500, e20e898, 9999144, cc36b64, 1941cdf, 05c02bb, 5706686, c668acb, fb5ea93 Since the previous push: ratio 0.981 -> 1.133, share 1.674 -> 1.449, rate 65631 -> 77251 (+17.7%). One push of difference sits inside this test's measured spread - read it as movement, not as a result. Updated for |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #428 +/- ##
============================================
+ Coverage 81.91% 82.45% +0.54%
- Complexity 1466 1479 +13
============================================
Files 95 95
Lines 5148 5165 +17
Branches 502 505 +3
============================================
+ Hits 4217 4259 +42
+ Misses 732 709 -23
+ Partials 199 197 -2
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:
|
|
@claude review this |
|
Claude finished @astubbs's task in 2m 37s —— View job Review
I read Correctness
Tests
Docs
Nothing correctness-blocking found. The self-review process described in the PR body (mutation-proving each new assertion, catching the DEBUG-dump-outside-the-guard defect and the dropped-throwable gap before this even reached review) already closed the issues I'd normally flag. Two very minor, non-blocking observations:
|
…ually need `bin/check-pr-analysis-surfaces.sh 428` reported one finding on a line this PR wrote, and nothing inherited: fb-contrib's overly-concrete-parameter rule on `linesMentioning`, whose body only calls `stream()`. Widened to `Collection`, and `only` with it - generalising the inner helper alone would have moved the finding to the caller that hands it the list rather than removed it. The `static: spotbugs` lane reports and does not gate (`docs/inflight/static-spotbugs-rule-registry.md`, "The lane REPORTS, it does not BLOCK"), so this is not a red build being cleared; it is the one item the surfaces script attributes to this diff, taken rather than argued. Behaviour unchanged - `ConsumerOffsetCommitterAsyncFailureLoggingTest` (2) and `RecordBatchSummaryTest` (13) green.
…of measuring it Review's one actionable observation on this PR: the `< 300` guard was a number read off a run, while its sibling in `RecordBatchSummaryTest` (`commitSummaryCostPerPartitionDoesNotDependOnMetadataSize`) derives the same kind of bound from what the rendering emits. So a reader could not tell what 300 was protecting, and the coupling to `TOPIC`'s length was invisible - the test would have started failing for an unrelated reason had the constant been renamed to something longer. Now `2 * (TOPIC.length() + 64) + 64`, with the terms named at the assertion: 64 per entry beyond the topic name covers `-<partition>: offset <offset>, <length> chars of metadata; ` even at a 10-digit partition, a 19-digit offset and a 4-digit length - the same allowance and the same reasoning the sibling test uses - and 64 more covers the statement's own fixed text. It STRENGTHENS the assertion rather than restating it: 284 against the previous 300, on a line that measures 211. The number that matters is the one it is nowhere near - metadata.length(), 4096, which is what interpolating the map put on this line and what the guard exists to keep off it.
Review round handledThe review found nothing correctness-blocking and opened no inline threads, so there is nothing here to 1. Now It strengthens the assertion rather than restating it: 284 against the previous 300, on a line that 2. The 3. One finding from Nothing was declined and nothing is left open for the owner. The one red check that is not this PR's
|
Takes #430 (5ae7b34), which excludes CVE-2026-82596 on org.latencyutils:LatencyUtils 2.0.3 - the advisory OSS Index started reporting on 2026-09-03, transitive through micrometer-core, that turned `deps: whole-tree CVE scan` red on every open PR including this one. It was master state, never anything this branch introduced, so this merge is how it clears here rather than a second exclusion contending with 430's. Disjoint by file: 430 touches pom.xml and docs/inflight/deps-cve-backlog.md, neither of which this branch has ever opened. Both sides are already on the bz.stub package layout, so the rename-before-merge rule in AGENTS.md has nothing to do here.
Taking master turned `repo: hygiene` red here, and the gate is right rather than confused. `docs/inflight/deps-cve-backlog.md` arrived with #430 recording that CVE-2026-82596 "went red on every open PR the morning it was published (#427, #428, #429)", and `bin/check-branch-self-reference.sh` fires on any note naming the PR whose state is about to change - which, on this branch, is #428. The sentence needs no rewrite: it is past tense about the morning of 2026-09-03 and reads correctly once this merges. What was missing is the attestation the gate exists to force, so it is added on the line rather than above it, which would have split the paragraph. This is the intended use of the marker, not a way round the gate: the gate scopes to whichever branch is merging precisely so the PR being named is the one asked to confirm. #427 and #429 are named in the same sentence and will hit the identical failure when they take master; whoever merges first gives master the marker and the other two inherit it.
…merge-correct Merging master brought in #430's backlog entry, which names the three PRs the advisory went red on - this one among them. `bin/check-branch-self-reference.sh` reads its `#NNN` arm over whole files, not just lines the branch added, so a line master wrote about #427 fails on #427's own branch. That is the gate working as designed: it fires only on the state about to change, and mine is. Marked rather than rewritten, which is the judgement the marker exists to record. The sentence is a dated record in the past tense - "It went red on every open PR the morning it was published" - and it names PRs, which the gate's own header calls permanent and fine to cite forever. What rots is a present-tense claim about a branch, and there is none here. It reads identically after any of the three merge. The attesting comment sits inside the checked block on purpose: it names #427 itself, so outside the block it tripped the same arm on its own first run. #428 and #429 will hit this identically when they merge master. This lands the marker once, on the first of the three, so the other two inherit it rather than each attesting the same line.
…merge-correct What changed for a user: nothing - a one-line marker in an inflight note. Merging master for #430's CVE exclusion brought in a sentence in docs/inflight/deps-cve-backlog.md naming the three PRs the advisory went red on, this one among them. check-branch-self-reference.sh reads whole files for the PR-number arm, so an inherited line naming #429 fails on this branch even though #430 wrote it - the gate only ever looks at the branch about to change state, and after this merge that is this one. The sentence is already written in post-merge terms: it is past tense about the morning of 2026-09-03, and it stays true once this PR lands. That is precisely the case the gate's header calls permanently fine - "PRs are permanent, and citing one is fine forever"; what rots is a deleted branch or a present-tense claim, and this is neither. So the attestation is the correct response rather than a rewrite, and it is added inline so the paragraph is not split. #427 and #428 inherit the same line and will hit the same gate on their own merge of master. The marker is not branch-specific, so whichever lands first carries it for the others.
…ng branches 34dde7f attested the LatencyUtils entry with the block form (`checked-begin`/`checked-end`) plus a comment explaining the judgement. #428 and #429 had already reached the same conclusion about the same inherited line and used the inline form - the marker appended to the end of line 199 - so three branches were about to land three different renderings of one attestation and conflict with each other over it. The gate accepts both, so this is not a correctness fix; it is choosing the form that merges. The file is now byte-identical to the copy on both sibling branches, verified by diffing against `origin/fix/168-commit-error-line-keeps-identifiers`, so whichever of the three lands first the other two see no conflict here at all. The reasoning the block comment carried is not lost - it is in 34dde7f's body, which is where prose reasoning belongs and which the release-note generator reads. In short: the sentence is a dated past-tense record naming PRs, the gate's own header calls citing a PR permanent and fine, and what rots is a present-tense claim about a branch, of which there is none here. Marked rather than rewritten, deliberately.
…esidentIT setup-guard flake What changed for a user: nothing - a ledger entry. The integration lane went red on this branch with one failure in 201 tests: RegistrationRaceStaleResidentIT.freshArrivalCollidingWithStaleShardResident\ MustStillGetProcessed, on "control thread must reach the mid-loop pause point (offset 25)". The ledger already carries that test from a single 2026-09-01 sighting on #257, with the same assertion and the same reading - the saturation/pause-point setup guard timing out, so the confluentinc#909 assertion the IT exists for is never evaluated. Why it is master-state and not this branch's: the identical failure hit feat/225-pc-built-producer at e35db1e seventeen minutes earlier, and four other branches passed the same lane inside the same 25 minutes (#428, fix/422-commit-interval-unset-detection, optimize/chaos-ci-perf, feat/225-producer-config). Two unrelated branches failing and four passing in one half-hour rules out both "this diff caused it" and "the runner was slow". Mechanism clears this branch independently, which is the part that actually settles it: the IT is PERIODIC_CONSUMER_SYNC driving pc.poll(...), so it never produces, never takes a produce lock, and never reaches the InvalidPidMappingException arm, ProducerManager#close or innerDoClose that this PR changes - and it fails in stage 2 of its own setup, before any close. Recorded rather than diagnosed or fixed, per this ledger's rule and the no-retry rule: the evidence expires with the logs, and the failing runs' 30.8s and 30.9s against 14-19s on the passing ones is the shape of a 30s budget waited out in full. Not quarantined - quarantine is master-state with evidence, and that call is not this PR's to make.
…r-line-keeps-identifiers
…per when #116 merged #216 asks for gauges over the buffers that grow when the user stops draining them, and had no note in this directory - so a session scanning docs/inflight/ saw nothing, and the reasoning lived only on the tracker. The note deliberately does not restate the issue. What it carries is the part the issue cannot know: three of the four premises it reasons from stopped being true when #116 merged, and each moved in the issue's favour. The backing collection is a LinkedBlockingQueue rather than a ConcurrentLinkedDeque, whose size() is a maintained counter, and the field's own javadoc names the gauge as why it was chosen - so the issue's central design caveat about keeping a counter alongside the collection is dissolved. The close-time WARN a comment describes as "what is left in its place" never shipped; the merge removed it with the other symptom mitigations, so there is now no signal at all between the backlog growing and the heap ending. And the JStream API is no longer deprecated, so the argument that a repair could not earn its place on an API being deleted has lost its premise. Still true, and stated so nobody reads the above as the issue being resolved: the queue has no capacity by design, so a consumer merely slower than the producer still grows it, and observability remains the only mitigation available. Both claims are cited to sources that state them from their own side - the javadoc on userProcessResultsStream and #116's squash commit - rather than asserted here, because this note goes stale the moment either changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0132Mx5qpSYmhDEref6Fgxjn
🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its No quarantined test changed outcome since the previous push. Updated for |
…, and the condition both share The integration lane went red on this branch with one failure, repetition 1 of 5 of rapidToggleShouldNotCreateDuplicateInstances, consumeCount 0 and the probe clean with detector reach UNKNOWN. The ledger already carried one sighting of exactly that shape, so this is the second and it gets recorded before this PR merges, because the evidence expires with the logs. Cleared on mechanism rather than on timing, the way the first sighting cleared #207: this branch's whole main-code change is one argument on the commit-failure ERROR line plus a DEBUG line beside it, both inside the exception guard of the async callback, and the symptom is zero records consumed after toggling an instance. What the second sighting adds is the condition rather than the count. Both fired on the first run of a branch that had just taken a change to how this lane runs - pr-integration-gate-wall-time.md already records a first-ever timeout failure in this same class when forkCount went 4 to 6, and this run was the branch's first under the newly sharded integration and chaos lanes. That makes it a load-shape canary rather than a random flake, which is a sharper claim than one sighting could support and points at the fix the existing entry already names: every wait in the test is a fixed sleep, so the sleeps are what lose when the runner gets busier. The same head was re-run once and passed, which separates always-red from not-always-red and establishes nothing about the rate - the method that solutions doc owns, cited rather than restated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0132Mx5qpSYmhDEref6Fgxjn
… it is The upstream-pr-log-noise entry still said `pr-open`, which was true of #428 until the moment it lands and false immediately after. The merge gate refuses on exactly that, and it is right to: branch content is invisible to everyone until it merges and correct the instant it does, so writing this afterwards means a commit straight to master, which is the stale-manifest failure docs/upstream.md describes and which this same entry already had to correct once for #203. The status is `merged` rather than `released` because nothing is published yet. It describes the fork work that has shipped, not the family being closed: the confluentinc#57 umbrella across core's debug and trace call sites is untouched, and the deferral WARN siblings in the same class wait on #352, both of which the entry's notes already carry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0132Mx5qpSYmhDEref6Fgxjn
1 participant
Closes #168 (mirror of confluentinc#629).
Description
The ERROR line for a failed asynchronous offset commit (
ConsumerOffsetCommitter, anchorError committing offsets: {}, exception:) interpolated the wholeMap<TopicPartition, OffsetAndMetadata>.The identifiers #168 asked for were already on it -
confluentinc#850 added them - but
so was each partition's
metadata: PC's base64-encoded offset map, at up toOffsetMapCodecManager.DefaultMaxMetadataSize(4096) characters per partition. So the line grew topartitions x 4KB and log tooling truncated it on exactly the occasion an operator needed it whole.
docs/inflight/bug-unbounded-log-lines.mdnamed it as the next instance after#203, with the constraint this PR follows: keep every identifier, reduce only the
metadata.
Before / after
Every partition is named - deliberately no partition cap, unlike the batch summaries in the same class: a
commit map holds one entry per partition, so the per-partition cost is fixed, and the partitions are the point
of the line. The metadata's length stays because it is the diagnostic when a commit is rejected for its
size. The leader epoch goes because PC never sets one.
How
RecordBatchSummary.summariseCommit- the shared renderer from #203, extended ratherthan paralleled; its comparator is generalised so both renderings sort by topic then partition. One argument
changes on the ERROR line and one DEBUG line is added after it. No
isDebugEnabled()guard is needed: theargument is a reference and SLF4J defers
toString(), unlike #203 where buildingthe argument walked the batch.
The callback's behaviour is untouched: the
todo keep work in limbobeside it belongs todocs/inflight/bug-async-commit-marked-successful-before-broker-ack.md, a different bug, and is left wherethat note can find it.
Tests
They assert the emitted line through
LogCapture, not the format string, so re-interpolating the mapfails a test:
RecordBatchSummaryTest- exact rendering with three partitions inserted out of order, the single-partitionand empty shapes, and a per-partition bound under Kafka's longest legal topic name with maximum-length
metadata on every partition of a large assignment.
ConsumerOffsetCommitterAsyncFailureLoggingTest(new) - drivescommitOffsetsinPERIODIC_CONSUMER_ASYNCHRONOUSagainst a mockedConsumerManager, captures the callback and completes itwith the reporter's
RebalanceInProgressException. Asserts the ERROR line names both partitions and theiroffsets, does not contain the metadata or
OffsetAndMetadata{, and stays short; and that the DEBUG linestill carries the metadata. A second case completes the callback with
nulland asserts nothing at ERROR,which is what stops the first case's negative assertions being vacuous.
Prove-it-red. The committer test was written and run against master's line first: it failed on
contains(<topic>-0: offset 1000, 4096 chars of metadata), and the captured line in the failure messagecarried the whole 4096-character metadata string - the defect, in the test's own output. It went green on the
fix. Mutating the fix once, by dropping
.sorted(...)fromsummariseCommit, fails the three-partitionrendering test; restored.
What review found
A local
ce-simplifyandce-code-reviewpass ran over this branch before it was opened. Simplify found onereal item:
summariseCommitshould short-circuit the single-partition case before the sort and join, the shapeits sibling
summariseOffsets(Map)already takes. Review ran seven reviewers and found five more, all applied,each proved by mutating the production code and watching the suite fail:
strongest signal in the run). Both log statements sit behind one
exception != nullguard, so moving orduplicating the unabridged dump outside it would put the whole offset map on the hot path of every
successful commit with the entire suite still green. That is the exact defect this PR exists to remove,
reachable without a single test going red. Now asserted; the mutation fails it.
LogCapture.messagesAt()formats themessage and drops the throwable, so deleting the trailing argument left every assertion passing. Now asserted
through the
events()accessorLogCapturealready exposed.names, which
summariseCommitdeliberately does not.LogCapturestates and this class's own comment claimed to follow.upstream-map'sfork.statussaidpr-openabout nothing - fix(logging) astubbs#169 astubbs#170: bound the dropped-batch and user-function-failure log lines #203 had merged andthis branch had no PR yet.
Two reviewer observations were not acted on, and are recorded rather than silently dropped: the
single-partition emitted line is untested (that rendering is already covered directly in
RecordBatchSummaryTest, so the extra case would test the summariser twice), andsummariseCommit'szero-partition branch has no production caller (
commitOffsetsreturns atNothing to commitfirst) - pinningthat early return belongs to a test about the caller, not this one.
Two test hazards found on the way to green, both recorded at the annotations
Neither was predicted. The module sets
junit.jupiter.execution.parallel.mode.default=concurrentbut leavesmode.classes.defaultat JUnit'ssame_thread, so the methods of one class run in parallel while classesdo not - which is what decides how load-bearing each guard is:
LogCapture's level restore, and this one actually happened.Whichever closes first puts the logger back to
INFO, and the other's DEBUG line is then never appended - theDEBUG assertion read an empty list. These are two methods of one class, so
@Execution(SAME_THREAD)is thefix, exactly as
LoadFactorCeilingReportingTestalready does for the same reason ("@isolated separates thisclass from others, not its own two capturing methods from each other").
@Isolatedis not warranted: itseparates classes, which already do not overlap, and this logger is quiet anyway.
OffsetMapCodecManager.DefaultMaxMetadataSizeis a mutable static thatOffsetEncodingBackPressureTestand its unit sibling write. Every test here that reads it takes the READ side of
METADATA_DATA_SIZE_RESOURCE_LOCK, the lock those two already hold for writing - asWorkManagerOffsetMapCodecManagerTestandOffsetEncodingTestsdo. Belt-and-braces rather than load-bearingwhile classes run sequentially, and the comment says so rather than implying otherwise.
Checked the rest of the class - one sibling found, recorded, handed on
The two
Offset commit deferred (postponed, not dropped)WARN lines - the sync-mode path for the same twoexceptions - carry no partition or offset at all. This also contradicts what the #168
mirror body currently claims ("No equivalent gap on the sync path"). The offsets are a local one frame down in
AbstractOffsetCommitter.retrieveOffsetsAndCommit, so the clean fix needs a field -and #352 is rewriting that method and already adds
lastAttemptedOffsetsfor exactlythis reporting. So it is recorded in
docs/inflight/bug-deferred-commit-warn-names-no-offsets.mdand handedthere, rather than two PRs contending over PC's most carefully reasoned method. The note names the rendering
to use, so the two lines end up saying the same thing in the same shape.
Also checked and dismissed: a SASL failure on the sync path is retried inside
ConsumerManager.commitSyncandnever reaches either line (its own WARN has the same gap, and is named in that note); on the async path it
arrives as
RetriableCommitFailedExceptionand the fixed line covers it.The wider defect class - a
WARN/ERRORinterpolating an unbounded collection - was re-swept acrossparallel-consumer-core/src/mainwith the note's own command, and no instance beyond the ones it alreadyrecords was found. The one candidate that reads like a hit is
AbstractParallelEoSStreamProcessor, anchorFailed to commit offsets during revoke: {}: it interpolates adescribeWithRootCausestring, not a collection, so it is bounded.PartitionStateManager'sPartitionStateline remains the class's only open instance, and it stays in the note.Also in this PR
upstream-map.yaml'supstream-pr-log-noiseentry still recorded fix(logging) astubbs#169 astubbs#170: bound the dropped-batch and user-function-failure log lines #203 aspr-openalthough it merged on 2026-09-02 - corrected while adding this work to the same entry, andMissing topic and offset infos when login error in ConsumerOffsetCommitter confluentinc/parallel-consumer#629 moved from
relatedtoissuesbecause the fork is now acting on it.docs/inflight/bug-unbounded-log-lines.mdloses this instance; thePartitionStateManagerone remains thenext, and is now the only one.
docs/inflight/issue-response-168.md- the draft reply for confluentinc#629: Missing topic and offset infos when login error in ConsumerOffsetCommitter #168, perdocs/inflight/AGENTS.md. Not posted, and it does not get deleted with this PR.Riding along: a note for #216
Unrelated to the log line, and here because it is a doc-only file that does not deserve a PR of its own.
docs/inflight/core-216-unbounded-buffer-gauges.mdgives #216 the in-flight note it never had, so a session scanningdocs/inflight/finds it rather than needing to think of querying the tracker.It deliberately does not restate the issue. What it records is the part the issue cannot know: three of the four premises it reasons from stopped being true when #116 merged, and each moved in the issue's favour. The backing collection is now a
LinkedBlockingQueuewhosesize()is a maintained counter, and the field javadoc names the gauge as the reason it was chosen, so the issue's central design caveat is dissolved. The close-time WARN a comment on the issue calls "what is left in its place" never shipped, so there is no signal at all between the backlog growing and the heap ending. And the JStream API is no longer deprecated, so the argument that a repair could not earn its place on an API being deleted has lost its premise. Both claims are cited to sources that state them from their own side rather than asserted in the note.Checklist
docs/inflight/bug-unbounded-log-lines.md, newdocs/inflight/core-216-unbounded-buffer-gauges.md, newdocs/inflight/bug-deferred-commit-warn-names-no-offsets.md, newdocs/inflight/issue-response-168.md,upstream-map.yaml;CHANGELOG.adocdeliberately untouched perAGENTS.mddocs/features/- N/A - the shape of an error line is not a featuredocs/inflight/working note (pr-/branch-) started at the PR's first commit - N/A - nothing here thatghcannot show; the finding that outlives this PR has its ownbug-note, and the issue reply has its own draftce-simplifyandce-code-reviewlocally - both ran; findings are in the What review found section above. Simplify found one real item (the single-entry short-circuit); review found five, the strongest being a mutation that leaked the whole offset map on every successful commit with the suite green. All were applied and each was proved by mutation.🤖 Generated with Claude Code
https://claude.ai/code/session_0132Mx5qpSYmhDEref6Fgxjn