Repository navigation
Conversation
astubbs
commented
May 6, 2022
| </dependency> | ||
|
|
||
| <!-- Build dependency --> | ||
| <dependency> |
Owner
Author
There was a problem hiding this comment.
possible to use old path version instead?
astubbs
added a commit
that referenced
this pull request
Apr 16, 2026
Root-causes rather than excludes. Two categories of real test bugs that PIT's slower instrumented JVM and different test ordering surfaced: 1. Broken teardown override hiding base-class cleanup. ParallelEoSSStreamProcessorRebalancedTest overrode @AfterEach close() with an empty body. Each parameterised invocation created a new ParallelEoSStreamProcessor that was never closed and never triggered the base class's Awaitility.reset(). By invocation #2, accumulated resource pressure broke timing. Fix: delete the override so the base class's close() runs. 2. Standalone tests skipping base-class cleanup + tight margins. MockConsumerTestWithSaslAuthenticationException and ProducerManagerTest don't extend AbstractParallelEoSStreamProcessorTestBase, so they got no Awaitility.reset() or pc cleanup. The SASL test additionally mutated the global (shaded) Awaitility default timeout without resetting. Fixes: - MockConsumerTestWithSaslAuthenticationException: scope timeout locally (atMost(90s)) instead of setDefaultTimeout, and add @AfterEach closing parallelConsumer. - ProducerManagerTest: add @AfterEach with Awaitility.reset(); bump commitLockAcquisitionTimeout 2s→10s; give the failing test's three bare await() calls and BlockedThreadAsserter explicit 20s timeouts instead of the 10s default (too tight under PIT). 3. PCMetricsTest.metricsRegisterBinding - two bare await() calls (lines 94, 180) used Awaitility's 10s default, too tight for 1500 records through PC's pipeline under PIT instrumentation. Bumped to 120s to match the atMost budgets already used elsewhere in the same method. Also drops -DexcludedTestClasses from the PIT maven command now that all four tests pass their baseline. Base-class @AfterEach Awaitility.reset() stays as a general leakage guard. Verified all four classes' tests pass locally under Surefire (10 tests, 0 failures, 1m 19s). PIT baseline will be verified by CI. Closes #39 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Apr 20, 2026
Root-causes rather than excludes. Two categories of real test bugs that PIT's slower instrumented JVM and different test ordering surfaced: 1. Broken teardown override hiding base-class cleanup. ParallelEoSSStreamProcessorRebalancedTest overrode @AfterEach close() with an empty body. Each parameterised invocation created a new ParallelEoSStreamProcessor that was never closed and never triggered the base class's Awaitility.reset(). By invocation #2, accumulated resource pressure broke timing. Fix: delete the override so the base class's close() runs. 2. Standalone tests skipping base-class cleanup + tight margins. MockConsumerTestWithSaslAuthenticationException and ProducerManagerTest don't extend AbstractParallelEoSStreamProcessorTestBase, so they got no Awaitility.reset() or pc cleanup. The SASL test additionally mutated the global (shaded) Awaitility default timeout without resetting. Fixes: - MockConsumerTestWithSaslAuthenticationException: scope timeout locally (atMost(90s)) instead of setDefaultTimeout, and add @AfterEach closing parallelConsumer. - ProducerManagerTest: add @AfterEach with Awaitility.reset(); bump commitLockAcquisitionTimeout 2s→10s; give the failing test's three bare await() calls and BlockedThreadAsserter explicit 20s timeouts instead of the 10s default (too tight under PIT). 3. PCMetricsTest.metricsRegisterBinding - two bare await() calls (lines 94, 180) used Awaitility's 10s default, too tight for 1500 records through PC's pipeline under PIT instrumentation. Bumped to 120s to match the atMost budgets already used elsewhere in the same method. Also drops -DexcludedTestClasses from the PIT maven command now that all four tests pass their baseline. Base-class @AfterEach Awaitility.reset() stays as a general leakage guard. Verified all four classes' tests pass locally under Surefire (10 tests, 0 failures, 1m 19s). PIT baseline will be verified by CI. Closes #39 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 of 2 tasks
This was referenced Jul 28, 2026
astubbs
added a commit
that referenced
this pull request
Jul 29, 2026
The docstring claimed the test verified the #57 OffsetMapCodecManager caching as well as the confluentinc#859 Set de-dup, but it only guards the de-dup. Confirmed by experiment: reverting the caching (Set intact) leaves it green, while reverting Set->List makes it fail. The caching effect is masked - the Set already collapses the duplicate registrations, and PartitionState constructs its own per-assignment OffsetMapCodecManager registering the same fixed-id meter regardless. Rewrite the docstring to scope it to the confluentinc#859 de-dup and explain why the caching cannot be asserted here; record in docs/refactoring.md that the caching lacks isolated coverage (no construction seam; redundant for the leak). Doc-only; test still green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5 tasks done
This was referenced Aug 3, 2026
4 tasks done
astubbs
added a commit
that referenced
this pull request
Aug 7, 2026
The reference gate requires any #NNN below the threshold to name its repo, because the fork's numbering sits entirely inside upstream's range and a bare number is a coin flip. It checks added lines only, so it never fired on text nobody was editing - leaving the convention true of the files the original work touched and false of the rest of the tree. Two passes, on the same lines and so landing together: - 366 bare references gain their repo. All 77 distinct numbers were resolved against BOTH repos first: 62 of them exist in each, meaning different things, so the classification is per-occurrence rather than per-number. #188 and #195 are fork mirror issues in the release notes while confluentinc#188 and confluentinc#195 are the upstream bugs cited in test comments in the same tree. - 24 `upstream #NNN` uses become the owner form. That form passed the gate but names a relationship rather than a repository, and this fork is itself upstream to anyone who forks it. Three sets are deliberately NOT prefixed, because they are not references: author ordinals ("run #1", "produce #1/#2", "NUDGE #1/#2") annotating log excerpts, reworded to plain numbers; the changelog gate's fixture, which asserts that a *bare* #NN is not a citation and would have been destroyed by qualifying it, so it moves above the threshold as a fake #999104; and upstream-pr-analysis.adoc, which is exempt and written entirely in upstream terms. README link text keeps its qualifier even though the URL beside it already names the repo - the gate can see a link target, a reader cannot, and this fork has its own #12. Quoted upstream titles keep the quotation intact with the number appended rather than having the owner inserted mid-title. README is generated: the edit is in src/docs/README_TEMPLATE.adoc. @tag("#355") becomes @tag("confluentinc#355"). Verified nothing selects on that tag - no pom, workflow or script filters it - and both classes still collect and pass. The 14 upstream-derived Java files gain the "Modifications Copyright" line the provenance-aware header check requires of any file changed since the fork point. docs/inflight/next-qualify-remaining-refs.md is deleted: this is everything it tracked, and in-flight files do not outlive their work. No behaviour change.
astubbs
added a commit
that referenced
this pull request
Aug 7, 2026
The reference gate requires any #NNN below the threshold to name its repo, because the fork's numbering sits entirely inside upstream's range and a bare number is a coin flip. It checks added lines only, so it never fired on text nobody was editing - leaving the convention true of the files the original work touched and false of the rest of the tree. Two passes, on the same lines and so landing together: - 366 bare references gain their repo. All 77 distinct numbers were resolved against BOTH repos first: 62 of them exist in each, meaning different things, so the classification is per-occurrence rather than per-number. #188 and #195 are fork mirror issues in the release notes while confluentinc#188 and confluentinc#195 are the upstream bugs cited in test comments in the same tree. - 24 `upstream #NNN` uses become the owner form. That form passed the gate but names a relationship rather than a repository, and this fork is itself upstream to anyone who forks it. Three sets are deliberately NOT prefixed, because they are not references: author ordinals ("run #1", "produce #1/#2", "NUDGE #1/#2") annotating log excerpts, reworded to plain numbers; the changelog gate's fixture, which asserts that a *bare* #NN is not a citation and would have been destroyed by qualifying it, so it moves above the threshold as a fake #999104; and upstream-pr-analysis.adoc, which is exempt and written entirely in upstream terms. README link text keeps its qualifier even though the URL beside it already names the repo - the gate can see a link target, a reader cannot, and this fork has its own #12. Quoted upstream titles keep the quotation intact with the number appended rather than having the owner inserted mid-title. README is generated: the edit is in src/docs/README_TEMPLATE.adoc. @tag("#355") becomes @tag("confluentinc#355"). Verified nothing selects on that tag - no pom, workflow or script filters it - and both classes still collect and pass. The 14 upstream-derived Java files gain the "Modifications Copyright" line the provenance-aware header check requires of any file changed since the fork point. docs/inflight/next-qualify-remaining-refs.md is deleted: this is everything it tracked, and in-flight files do not outlive their work. No behaviour change.
This was referenced Aug 11, 2026
This was referenced Aug 18, 2026
astubbs
added a commit
that referenced
this pull request
Aug 25, 2026
…ts own seams The review's remaining two mechanical findings (#2, #11). MockConsumerCommitFailureSeamTest had grown to 1456 lines and 27 flat test methods, and PMD CPD/jscpd flagged its repeated setup and Awaitility blocks as this PR's largest new duplication. Now: MockConsumerCommitFailureSeamTestBase owns the fixture (failing MockConsumer factory, RecordingHandler, startPc overloads, latching feed, teardown, awaitAsserted and the CONTINUE opening helpers), and five classes own one seam each - Decision (10), HandlerFreeExits (4), PauseIntake (5), DeferralEscalation (5), Metrics (3). All 27 tests survive with assertions byte-for-byte intact; the base keeps the documented decision NOT to extend MockConsumerTestBase. ParallelConsumerOptionsCommitFailureTest's twin assertThrows blocks collapse into one assertAsyncRejects helper. The deliberate ConsumerManagerCommitRetryBudgetTest/ProducerManagerCommitBudgetTest mirror is untouched. Verified: 27/27 across the five classes plus options/symptom suites, maven exit 0; copyright, issue-ref and file-ref gates green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016jCtV2yqQvxDR6cLML8J8a
Merged
3 of 4 tasks
5 tasks done
astubbs
added a commit
that referenced
this pull request
Sep 2, 2026
…n class, and resolve the transactional id once per instance The structure findings #18, #14 and #30 of the #410 review; #2 is deferred to docs/refactoring.md with the reason. - The ledger moves out of PartitionState into a package-private UncommittedCompletions: the map, the in-commit snapshot and the monitor, with record, snapshotForCommit, onCommitSuccess, snapshotInOffsetOrder and forget, and a NONE instance chosen once in the constructor for the non-transactional modes - which deletes the per-completion mode branch retainsCompletedRecords(). Behaviour is unchanged and PartitionStateAbortedTransactionReplayTest is untouched and green. This is also the fix for the red `static: infer` CI lane: RacerD analyses a class once it contains a synchronized block, PartitionState gained one with the ledger, and the lane reported thirty of that class's unrelated accessors. The monitor now lives in a class whose every field access is under it, so there is nothing for RacerD to report there, and config/infer-known-findings.txt is not edited. The thread-safety invariant (control thread writes and replays, whichever thread commits trims, never held across a lock acquisition, client call or callback) is recorded on the new class per the engine's AGENTS.md. - PCModule resolves the producer configuration once and memoises it; the replacement source reads the derived transactional.id from that map instead of deriving it a second time, and every build receives a copy of the same map. TransactionalIdDerivation.derive() now has one caller in main code (resolve), and the WARN a caller-set id earns fires once per instance rather than once per rebuild - an operator watching a recovery loop sees the recovery lines, not a repeat of a start-up warning. PcBuiltProducerTest.aCallerSetIdIsWarnedAboutOnceAcrossAStartAndTwo Replacements counts 1; with resolve() back per build it counts 4. - The two PC-built InvalidPidMapping tests in ParallelEoSStreamProcessorTest shared a verbatim body (the one new PMD CPD clone on the PR); they are one parameterised test over the two arrival shapes, synchronous throw and failed send future, each still visible by name in the report. - Not done here: extracting the availability state machine from ProducerManager (finding #2). After groups 1 and 2 the machine spans the write-lock path (beginReplacement sets the replay-owed flag) and the produce-lock check (the dispatch-generation refusal), so the seam the finding proposed - "holds no lock but its own monitor, makes no client call" - is not a clean one-pass move any more. Recorded under internal/ProducerManager.java in docs/refactoring.md. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
6 tasks done
5 tasks done
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
1 participant
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.
No description provided.