feat(core) astubbs#225: derive the transactional.id, build the producer through a factory PC enforces, and redact its configuration - #420
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
✅ 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
|
| PR | Base | Change | |
|---|---|---|---|
| Clones | 111 | 111 | ➖ 0 |
| Duplicated lines | 1578 | 1557 | :face_with_raised_eyebrow: +21 |
| Duplication | 0.84% | 0.83% | 🫤 +0.01% |
| Rule | Limit | Status |
|---|---|---|
| Max duplication | 2% | ✅ Pass (0.84%) |
| Max increase vs base | +0.1% | ✅ Pass (+0.01%) |
⚠️ 2 new clones introduced
- 27 lines:
parallel-consumer-examples/parallel-consumer-example-reactor/src/test/java/bz/stub/parallelconsumer/examples/reactor/ReactorAppTest.java:60<->parallel-consumer-examples/parallel-consumer-example-vertx/src/test/java/bz/stub/parallelconsumer/examples/vertx/VertxAppTest.java:66 - 28 lines:
parallel-consumer-examples/parallel-consumer-example-reactor/src/main/java/bz/stub/parallelconsumer/examples/reactor/ReactorApp.java:27<->parallel-consumer-examples/parallel-consumer-example-vertx/src/main/java/bz/stub/parallelconsumer/examples/vertx/VertxApp.java:28
Powered by astubbs/duplicate-code-cross-check
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
[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. |
|
🟢 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: a37d148, e8bd2cb, 1743297, 4bc6e7a, b62c310, c381310, c79424a, 9c67c89, f1aa5eb, eb9fdb0 Since the previous push: ratio 1.056 -> 1.133, share 1.753 -> 1.604, rate 72420 -> 76925 (+6.2%). One push of difference sits inside this test's measured spread - read it as movement, not as a result. Updated for |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## docs/225-producer-fencing-brainstorm #420 +/- ##
==========================================================================
- Coverage 84.12% 84.08% -0.05%
- Complexity 1762 1798 +36
==========================================================================
Files 104 108 +4
Lines 5884 5956 +72
Branches 599 615 +16
==========================================================================
+ Hits 4950 5008 +58
- Misses 730 737 +7
- Partials 204 211 +7
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:
|
…as, and take the note off deferred The note said the work was deferred to after v6; it is in flight as #426 (this rung), #410 (recovery, stacked on it) and #420 (the rest of producer ownership, stacked on recovery). Absent state means open, so the deferred line goes and the stack is listed where the next reader looks first. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
…g-brainstorm Brings in rung 1 (#426): producerConfig with the default constructor, cut out of #420. Every conflict - the options, the module, the wrapper, the two option tests, the README template and its generated README, the in-flight note - resolves to this branch's version, whole file: each of them here is the #420 superset of what rung 1 carries, and the commit after this one re-cuts them all to the rung-1 shape. Nothing rung 1 adds is lost by that; it is re-applied there, by hand, where it now belongs. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
…he factory, the derived id, redaction and the deprecation up the stack This PR now stacks on #426 - producerConfig with the default constructor - rather than on #420. Everything #420 carries beyond that rung leaves this branch's tree here and returns above it, in #420 itself, which now stacks on this PR: ProducerFactory and its contract exception, TransactionalIdDerivation, ProducerConfigRedaction (and their tests), the deprecation of the instance option with its WARN and removal queue, the README ownership section and migration note, and the four migrated example apps, which go back to master's. What recovery is built on instead: the module's protected buildProducer(Map) seam from #426. replacementProducerWrap() builds another wrapper through it from the same configuration, the caller's transactional.id included, and hands the id to the ReplacementProducerSource so a failure to build can still name it. The build runs through UserFunctions.carefullyRun as before: the seam is overridable, and an Error from the constructor must surface as a build failure rather than escape the recovery pass. The policy's terminal set loses the factory-contract clause with the factory; Authorization, UnsupportedVersion and Error stay. Tests move with it. ProducerRecoveryTest hands ParallelEoSStreamProcessor a PCModule whose seam hands out the spied MockProducers the factory used to, records the id each build was handed, and asserts it is the caller's, unchanged, on every build; the log-line and failure-message assertions key on a per-instance id the test sets, as they keyed on the derived one. ProducerFencingRecoveryIT sets its own transactional.id so the rogue can fence under it, and drops the derived-shape assertion, which returns with the derivation. ProducerManagerRecoveryTest's factory-contract case leaves with the contract. The two option/module test classes are rung 1's. Records: the plan carries a dated re-cut note at its head rather than a rewrite of its units; the in-flight note lists the three-PR stack; the roadmap entry, the issue-response draft, the refactoring queue and the README template (a producer-recovery section only, no ownership section) say what this PR now says. Mechanically this commit is the inverse of what #420 becomes: reverting it on top of this branch is how #420 is re-based, so its history is kept too. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
…d move the factory, the derived id, redaction and the deprecation up the stack" This reverts commit 91f9e80, which is how this PR is re-based to stack ABOVE recovery (#410) rather than below it. That commit took the factory, the derived transactional.id, redaction, the deprecation of the instance option and the example migration out of #410's tree; putting them back on top of #410's head is exactly this PR's content, with its history kept: the original extraction (d01bad2) is still here, and this PR's diff against its base is the inverse of the re-cut. Two records are kept from #410 rather than reverted, because they describe the stack as it now is: the plan's re-cut note at its head, and the in-flight note's three-PR listing. What this PR is now, stated from this side: #426 gave PC producerConfig with the default constructor; #410 gave it recovery, rebuilding under the id the caller set; this PR gives PC ownership of the id (derived, prefix-free, per instance), an overridable ProducerFactory whose contract PC enforces, redaction of configuration values, and the deprecation of the instance option with the examples moved off it. The seam #426 added, PCModule.buildProducer(Map), is subsumed by the factory here. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
…he stack has merged It names the branches the sightings came from, in the past tense, which stays true after they land; the self-reference gate on #420 asked for the attestation. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
[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. |
…built-producer Carries rung 1's review fix (#426) up through recovery. Resolved by hand in PCModule: the factory path already closed a producer the construction check rejected, but caught only RuntimeException, so the wrapper's reflective failure on a KafkaProducer subclass - a checked exception thrown sneakily - slipped past it exactly as it did on rung 1. That catch is now Throwable, through one closeQuietly, and the rung-1 test is rewritten for the factory path: a factory returning an anonymous KafkaProducer subclass, asserting the producer was closed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
[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 Since the previous push: 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 Since the previous push: 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. |
…dentIT setup-guard failure On #420's head, same guard, probe clean, passing on the heads before and after; the diff between them was a test assertion in another class. Recorded for the count. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
…built-producer Carries rung 1's close-failure test up; rewritten for the factory path, as the other rung-1 module tests were. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
…uild latch is holdBuildUntil #410 was re-cut to stack on #426: the producer factory moved up its stack into #420, and ProducerRecoveryTest now hands PC a module whose buildProducer seam hands out the mock producers. The revoke-path fence test held the replacement build on the old holdFactoryUntil latch; it is holdBuildUntil now, and nothing else about the test changes. Also cleared: the module's stale generated Truth subjects for the moved ProducerFactory made the generator fail with "Type ... not present" until target/ was cleaned - a clean-then-verify, not a source change, recorded here because the error reads as a broken tree. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PVL2FEJ645T76PbybEBUZ6
…built-producer # Conflicts: # parallel-consumer-examples/parallel-consumer-example-vertx/src/main/java/bz/stub/parallelconsumer/examples/vertx/VertxApp.java
…-correct Both arrived with master: the consumer-half note names this PR as the producer half whose reasoning transfers, and the churn-storm ledger names this branch in a dated sighting. Each reads as a record once this lands, which is what the gate asks a branch to say about itself. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
[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. |
…fter it Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
[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 Since the previous push: 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 Since the previous push: Updated for Superseded by a newer quarantine lane report. |
…m this branch already uses The merge of #410 brought the groundwork rung's three module tests in their rung-1 shape (a caller-supplied transactional.id, the rung-1 helpers). On this branch the id is derived and the module is built through the factory, so the tests are re-expressed with this file's helpers: the derived id is asserted by prefix, and every build is shown to receive the same one. The instance-path case was already covered here and is not duplicated. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
[superseded - the quarantine lane is now empty] 🧪🔒 Quarantine Lane ReportThe superseded report, collapsed because it no longer applies
🔴 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. |
…left behind Moving the reactor and vertx examples onto the configuration path added an import block on top of one that already imported the same type, so each file ended up importing java.util.Map twice, and VertxApp imported JStreamVertxParallelStreamProcessor twice as well. Nothing fails on it - javac accepts a repeated single-type import, -Xlint:all does not warn, and the examples compile and their tests pass either way - so it would have merged silently and stayed. Removed the added copy in each case, leaving HashMap and Map in the order the rest of the file uses. No behaviour changes; test-compile of both example modules and their dependencies is green, and bin/check-all.sh passes. Swept the whole tree for the same defect class rather than the two files the duplicate-code bot named. Four files carry a repeated import; two are these, introduced by this branch and fixed here. The other two are inherited and deliberately left alone: WorkManager's ThreadConfined, added by the base rung #410 in f0af594 and f6bc1f8, and CommitHistorySubject's two truth-generator imports, which predate master's fork point. Neither is on a line this PR wrote. Nothing enforces this - no gate, no compiler warning - which is why the sweep found instances on two branches at once. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
…nder it grew to five ab86e34 rewrote the list in this section for today's re-cut - #472 and #474 cut out from under this PR - and left the heading above it saying "three PRs (2026-09-03)". A reader who trusts the heading over the list it introduces gets the stack's shape wrong, and this note exists precisely to tell a later PR which rung it has to reconcile against. Verified against GitHub rather than the list: #426 MERGED, then #472 -> #474 -> #410 -> #420 all open, each based on the one before it. Five rungs, one landed, which is what the heading now says. The list's own contents were already correct and are untouched. Nothing enforces a heading against the block it introduces; grepped the tree and every ref first, as AGENTS.md requires before moving a labelled block - the string is cited nowhere, it only appears as this heading on the branches that carry their own copy of the note. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
|
@claude review this |
|
Claude finished @astubbs's task in 5m 1s —— View job Review: PC-built producer, derived
|
…thdrawn, rejoin is expressible Two of the burn-down's open items closed on 2026-09-08, each by its own agent, and the note records both as things that happened rather than things to do. The eager-mode PERIODIC_CONSUMER_SYNC stall (the confluentinc#857 family's fourth open item) is withdrawn by #478: four replays of the recorded seed on today's master drained completely with zero loss, and the "stall" was the Class 2 timing bound, whose crossing flips with the processor count at a fixed seed and tree. The grid it was opened on was also never a one-term A/B. Nothing to ship or to name; what survives is the per-shard liveness gap, already tracked and deferred. Whether "rejoin" after producer fencing is expressible in the lifecycle is known by a read of the #472/#474/#410 diffs against the engine's ownership rules: it is, and the question dissolves, because the instance never leaves the group - rejoin reduces to abort, replace and replay on the control thread under the write lock. The stack respects the @GuardedBy ledger, thread confinement and the lock pair. What it leaves is review-sized: #420's territory, one wire-level test nobody wrote, and the plan's one open question about declining the lock during a rebalance. None of it changes the tier 2 decision. Claude-Session: 460f7df9-dcc2-4b00-a9f9-62f3a2c6d5e4 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
…built-producer Brings the recovery rung's merge of the ledger rung, #472's review fix and the master those carried. Resolved by hand, all in the producer build: - #472's review found the start-up build wrapped as user code, so a configuration the client refuses reached the caller renamed. This branch had the same shape through the factory. Now the start-up build calls the factory directly and a replacement build runs it under carefullyRun, the way that rung split it - but the wrap covers the factory's own throw only: the null and reused-instance contract checks are PC's verdict on what the factory returned, and keep ProducerFactoryContractException on both paths. The first cut wrapped the whole build and turned that verdict into an ExceptionInUserFunctionException, which the contract tests caught. - The two tests that rung added for the split are re-expressed with this branch's factory helpers: a start-up build with no serializers surfaces the client's ConfigException, and a replacement build whose factory throws an Error surfaces it wrapped, which the policy reads as terminal. The instance-path case was already covered here. - ProducerRecoveryPolicy keeps ProducerFactoryContractException among the terminal build failures, which the base branch does not know. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
…rries, and a consumer-commit build needs no group id Three findings from the automated review of #420, each fixed where it was found: - The factory contract only caught a DROPPED transactional.id: the construction check compared transactionality, so a factory that substituted its own id passed, and a replacement initialised under it would fence nothing. A KafkaProducer can say which id it was built under, so ProducerWrapper.forPcBuilt now takes the id the map carried rather than a flag, and rejects a producer built under a different one, naming both. Other producer types cannot be read back; the factory javadoc now says exactly what is verified and what rests on the caller. - resolvedProducerConfig() read the consumer's group id unconditionally, as a plain argument, so every producerConfig build - the consumer-commit modes included, which derive no id - started to need group metadata the poll-path never required. A manual-assignment consumer, or an unstubbed test double, has none and failed with a message about transactional.id for a producer that was never going to be transactional. The id is now read only where one is derived. The example tests' group-metadata stubs are left as they are; they predate this and cost nothing. - The README's client-setup paragraph still told readers to set transactional.id by hand in the transactional mode, the opposite of what the derivation does. It now says PC sets it, that a set value is replaced at WARN, and points at the recovery section for the derivation. Each new test was run against the code before its fix: the substitution case threw nothing, and the no-group-metadata case failed on the derivation guard. Both pass with the fixes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
🧪🔒 Quarantine Lane ReportThe quarantine lane is empty - no Any earlier row on this PR asking for a Lane: non-gating; rules: see the Quarantine Audit check. No quarantined test changed outcome since the previous push. Updated for |
1 participant
Part of #225 - the rest of producer ownership, stacked on top of recovery. The instance option is kept as a peer (owner's call, 2026-09-03): nothing is deprecated; what needs a PC-built producer refuses an instance at validation instead.
depends on #410
Description
PC builds its producer through a factory. On top of
producerConfig(Map)from #426,ParallelConsumerOptionsgainsproducerFactory(...); the factory defaults tonew KafkaProducer<>(config)and a caller overrides it to wrap or instrument the producer, building from the map it is given unchanged. It subsumes the protectedPCModule.buildProducer(Map)seam the base rung added. The producer-instance option still works for every flow it worked for and is not deprecated: both ways are peers. Where they differ, validation says so -producerFactorywithoutproducerConfigfails naming both, andproducerFactorybeside aproducerinstance fails naming all three - rather than a factory that is silently never called. Recovery does not run on the instance path; that is documented, not warned.PC derives the
transactional.idin the transactional commit mode:pc-<L>-<group.id>-<uuid>,<L>being the decimal length of thegroup.id. The prefix is stable for the group and prefix-free between groups (appversusapp-x), so one prefixed TransactionalId ACL covers every id PC derives for a group; the suffix is a per-instance secure-random UUID reused by every producer PC builds for that instance. A caller-set id does not take effect and is reported once at WARN.Configuration values never reach a log line, an exception message or a
toString: rendering goes through an allow-list of non-secret keys and redacts every other value unconditionally.The factory contract is enforced at construction: a factory returning a producer it already returned (caching or pooling), or one that dropped the
transactional.id, is rejected naming the contract. A producer built for a manager that then fails to construct (a coordinator not yet reachable atinitTransactions) is closed rather than leaked per start-up attempt.Docs and examples: the README teaches the configuration path and carries a note on moving to it when recovery is wanted (ACL re-grant, tooling keyed on the id, wrapping through the factory); every example app uses the configuration path, with the same factory hook in the core, vertx and reactor examples so their tests substitute the producer PC builds.
Why this is above recovery, not below it
Recovery needs only to build another producer from the same configuration and to re-initialise it under the same id, which the caller's own id does - so it stacks on
producerConfigalone. What is here improves on that rather than enabling it: a per-instance derived id means two instances of one application can never share an id, and the factory contract is enforced because PC may discard a producer and build another. None of it changes concurrency.Proven able to fail
Every test class here was proven red against a mutant in #410's review rounds: the redaction leak, the caching factory, the pooling factory (A, B, A), the dropped id, the WARN-once rule for a caller-set id, the construction-failure close. Added since: the two validation refusals, and silence on the instance path. The core suite and all four example modules' tests are green on this branch.
Checklist
docs/features/-N/A - documented in the README sections above; no docs/features/ entry exists for the producer to extenddocs/inflight/working note (pr-/branch-) started at the PR's first commit -N/A - one atomic commit extracted from astubbs/parallel-consumer#410, whose in-flight note covers the whole of astubbs/parallel-consumer#225ce-simplifyandce-code-reviewlocally -N/A - this content had both passes, plus an automated review and a Codex review, as part of astubbs/parallel-consumer#410 before extraction🤖 Generated with Claude Code
https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg