Repository navigation
refactor(core) astubbs#159: ship LongPollingMockConsumer in the main artefact - #202
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Claude is reviewing this PR
|
✅ 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 |
Lifecycle transition: the entry went in as `ready` with no PR because the number does not exist until `gh pr create` returns. Advance it to `pr-open` with prs: [202], and add the merge + #40-unblock actions to its todo list, so `upstream-map.py todo` shows the loose ends rather than a manifest that claims the work has no PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
|
|
Claude finished @astubbs's task in 3m 43s —— View job Claude finished the reviewTodo list
ReviewThis is a clean, well-scoped, and unusually well-verified relocation PR. Summary of what I independently confirmed:
No issues found. The scope discipline is notable: a class-relocation PR that resists the temptation to also fix the One minor, non-blocking observation: the new javadoc on |
Review responseThanks - the independent re-derivation (rename detection, the pom-scope check, running the copyright scanner and its self-test, the targeted test run) is exactly what makes a "no issues" verdict worth something. Responding to the one observation, plus two things I found while re-checking the review's claims myself. 1. Javadoc /
|
…ng readers it blocks The in-flight note and the upstream-map entry both said fork #40 was queued behind this PR and should be unblocked "the moment #202 merges". That stopped being true: this branch edits none of the four MockConsumer* tests, so the collision never materialised and #40 went ahead on its own as PR #206, open now. Left as written, both files were a stale marker telling the next session to wait for something that already happened - exactly the failure mode docs/inflight/AGENTS.md warns about. Also trims the in-flight note down to what `gh pr view` and the diff cannot answer. The review flagged the package/SpotBugs reasoning as intentional triplication across javadoc, docs/refactoring.md and this file; the javadoc and refactoring.md are the right homes for it, so this file now states the decisions and points at them instead of restating them in full. While verifying the SpotBugs entry, noted that poll() and wakeup() are already synchronized methods that each open a further synchronized (this) block on the monitor they hold. Reentrant so harmless, but it is what makes the guard/notify pairing hard to read, and likely why the AtomicBoolean looked necessary - recorded with the deferred fix so it gets cleaned up in the same pass. Refs #159 · upstream confluentinc#526 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
|
Claude finished @astubbs's task in 1m 45s —— View job Claude finished the reviewTodo list
ReviewOnly one commit landed since the last review pass ( What it does, and what I independently checked:
No issues found. This is a good, self-critical example of a docs commit: two real defects were caught by re-checking the PR's own claims rather than waiting for a reviewer (the stale #40 marker was found by the author, not flagged in my prior review), and the fix is scoped to exactly the drift it corrects — no unrelated rewriting. |
Merged master - and it required two substantive changes, not just a mergeMerged Reading what landed mattered more than the merge. 1. The
|
| Check | Result |
|---|---|
bin/check-shell-sigpipe.sh |
ok - no bin/*.sh pipes into grep -q under pipefail |
bin/check-action-versions.sh |
ok - every action at a single version |
bin/todo-index.sh --check |
docs/TODO_INDEX.md is up to date |
scripts/upstream-map.py validate |
OK, 14 entries |
bin/check-copyright-headers.sh |
232 files, 0 violations |
bin/test-check-copyright-headers.sh |
all self-tests pass |
| issue-ref gate over this diff | 15 → 0 |
No source, test or build change in any of this - the merge and both follow-ups are documentation, one shell comment line, and the PR body. The relocation itself is untouched since the last review.
Master's 735b1d3 mirrored all 78 confluentinc issues into this fork, which made a bare `#NN` ambiguous: the fork numbers from 1 and confluentinc reaches confluentinc#922, so the ranges overlap completely. The house convention is now `#159` / `confluentinc#526`, and `.github/scripts/issue-ref-gate.js` enforces it on added lines via the PR Checklist job. This branch predates that and had 15 unqualified refs across three files - `#159`, `#40`, `#202`, `#206`, `#162`, `confluentinc#861`, `#526`, `#271`. Every one is now qualified. Verified by running the gate module itself over this branch's diff rather than by eye: 15 findings -> 0. `upstream #NNN` is still accepted by the gate, but converted anyway - "upstream" names a role rather than a repo, and this repo is upstream to anyone who forks it, which is why master moved off it. No wording changed beyond the references themselves, except the in-flight note, which now also records that #159 IS the upstream-mirror issue for confluentinc#526 - that is where the fork<->upstream mapping lives now that the map entry is gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
|
Claude finished @astubbs's task in 3m 13s —— View job Claude finished the reviewTodo list
ReviewOnly two commits landed since the last review round: Because the merge pulls in a large unrelated upstream history, I diffed against the actual merge base (
Independently verified, not just read:
No issues found. This round is exactly what it claims to be: a merge plus two small, correctly-targeted follow-ups (dropping the now-superseded |
docs/inflight/AGENTS.md is explicit - "Track only what is currently OPEN", and root AGENTS.md draws the line as "happening now -> docs/inflight/; should happen later -> refactoring.md". This note failed that on two of its three sections. The harness is not in flight; it lands with this PR. Removed, because a permanent home already owns it: - The design rationale (why it does not extend AbstractParallelEoSStreamProcessorTestBase, why assertions stay in the subclasses, why a scenario is a subclass rather than another copy) is in MockConsumerTestBase's class javadoc, next to the code it constrains. - The measured similarity figures and the deferred cross-module clone backlog are in docs/refactoring.md, which this note already linked to while also restating them in full - the duplication AGENTS.md warns drifts apart. What is left is the part that really is transient: this PR moves the wiring out of six files, so every branch that forked before it conflicts on them, and the resolution is "keep the harness, re-apply the scenario" rather than the reverse - re-applying a pre-harness copy silently un-does the extraction. Plus the #202 pointer. The file now says when to delete it.
The last section standing was a warning that branches forked before this one will conflict on the six MockConsumer files. That fails docs/inflight/AGENTS.md's own rule against writing down what a command can answer: git announces a conflict when you hit it, and gh pr list answers what #202 is doing. A note whose content the tooling volunteers is one more thing to keep true, not a service to the next reader. The one part no command supplies was the resolution - keep the harness, re-apply the scenario, rather than re-applying a pre-harness copy of the wiring and silently undoing the extraction. That is already in MockConsumerTestBase's class javadoc, which says the wiring lives there "so that a new rejection or outage scenario is a subclass rather than another copy of the wiring", and it is next to the code a conflict resolver is looking at rather than in a directory they would have to think to check. Nothing references the file. This leaves the PR's docs footprint as docs/refactoring.md alone, which is the permanent home for the deferred cross-module clone backlog.
Downstream projects that want to test against the parallel consumer need a Kafka MockConsumer that actually long-polls - the AK one returns instantly, so a control loop under test spins. We have that class, but it lived in src/test/java, so the only way to get it was a <classifier>tests</classifier> dependency on parallel-consumer-core. That is an unpleasant thing to ask of a user: test-jars are not transitive, Maven resolves them awkwardly (confluentinc#162), and they drag the whole test surface along. Move it to src/main/java so a plain dependency on parallel-consumer-core is enough. The package is untouched - src/test/java to src/main/java on the same bz.stub.parallelconsumer.internal.utils path. Nothing internal-only becomes public ------------------------------------ The obvious objection is that this is an internal-utils package and the class is now an API commitment. But that package is ALREADY in the main artefact - Range, JavaUtils, TimeUtils and seven others ship in the main jar today - so the artefact's public surface gains a class, not a package. The original version of this change carried a second argument that no longer holds and is deliberately dropped rather than replayed: that keeping the fully-qualified name made the downstream migration a pom edit with no code change - delete the tests-classifier dependency, keep the import. Master's io.confluent.* -> bz.stub.* rename has since broken every downstream import regardless, for reasons that have nothing to do with this class, so there is no FQN stability left to protect and no saving to weigh a rehome against. That inverts the deferral, and docs/refactoring.md's breaking-change entry now says so. A supported test-support class still does not belong in an internal-utils grab-bag - `internal` in the name now says as much out loud - and the right home is bz.stub.parallelconsumer.testing, or its own parallel-consumer-test-support module in the kafka-streams-test-utils mould. The old reasoning parked that behind the confluentinc#271 restructure to avoid migrating the same users twice. With 0.6.0.0 unreleased and already carrying the rename as a breaking change, doing the rehome IN 0.6.0.0 is one migration; it is now the deferral that costs a second one. Left as a queued decision rather than taken here, so this stays a relocation. It dragged nothing with it, and no dependency changed scope ----------------------------------------------------------- The class needs kafka-clients, unij, lombok and slf4j. All four are already compile scope, so parallel-consumer-core/pom.xml is untouched and no downstream user gains a dependency. In particular MockConsumer, MockProducer and the reflected SubscriptionState are all in the MAIN kafka-clients jar, not its test artifact - had they not been, promoting a Kafka test artifact to compile scope for every user of this library would have been too high a price and this change would not have been worth making. The only test-only reference was a javadoc @see on KafkaTestUtils, which would have become a dangling link in the published javadoc; rewritten as plain text, the same shape UnmailboxableRecordException already uses for the same reason. No consumer of the class needed editing - not the MockConsumer* tests, not the examples, not the Truth subject - because only the source root moved. What this does NOT do --------------------- It does not close confluentinc#162 / confluentinc#861. Every module that consumes core's tests classifier still needs it for KafkaTestUtils, AbstractParallelEoSStreamProcessorTestBase and LongPollingMockConsumerSubject - verified on this tree: all three are still under src/test, and the mutiny, vertx, reactor and four example modules still reference them across the classifier. The last of those is Truth-generator glue: moving it would promote com.google.truth from test to compile scope for every downstream user, which is exactly the cost this change was careful not to incur. The README's "mvn compile needs test-compile" note therefore still stands and is left alone. The SpotBugs claim is also retired rather than replayed ------------------------------------------------------ The original change recorded NN_NAKED_NOTIFY as a false positive newly *visible* because SpotBugs analyses main sources and the class was becoming one. Master's spotbugs-maven-plugin configuration now sets includeTests, so that is no longer true either: measured with the class still under src/test, `./mvnw -pl :parallel-consumer-core -am -DskipTests test-compile spotbugs:spotbugs` already reports NN_NAKED_NOTIFY at LongPollingMockConsumer.wakeup. The move changes the file's source root, not whether it is analysed. The finding and its fix stay recorded in docs/refactoring.md, with that measurement, and are deliberately not acted on here. Also registers the relocation in bin/check-copyright-headers.sh's RENAMED_FROM_UPSTREAM. PACKAGE_MOVES maps the package but not the source root, so without the entry the file resolves to a src/main fork-point path upstream never had, is judged fork-original, and its required Confluent header is reported as a violation - confirmed with a control arm: deleting the entry takes the check from 0 to 1 violation, naming this file. No upstream-map.yaml entry: the manifest maps upstream PRs, and #159 IS the upstream-mirror issue for confluentinc#526, so it already owns the mapping. The in-flight note says so rather than duplicating it. Verified: parallel-consumer-core unit suite green (533 tests, 0 failures, 8 skipped); main jar contains LongPollingMockConsumer.class and the tests jar no longer does (it keeps only LongPollingMockConsumerSubject.class); javadoc jar builds and publishes the class; check-copyright-headers, check-issue-refs, check-inflight-tags, check-file-refs, check-branch-self-reference, check-upstream-map, check-docs-data and todo-index --check all pass. Upstream-Issue: confluentinc#526 Forwarded: not-needed Applied-Upstream: no Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMeraeBL2ycXVHEaTHM86W
56c895c to
e3c3175
Compare
[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 Superseded by a newer quarantine lane report. |
|
@claude review this Context: this branch was re-cut onto today's master, not merged. Its previous head carried its own copy of the Two arguments the original PR rested on did not survive and are explicitly retired in the body. Please check I retired them correctly rather than over- or under-claiming:
Also worth a look:
Claim (a) in the body — that this does not eliminate the test-jar dependency — was re-verified on today's master: all three blocking classes still under |
|
Claude finished @astubbs's task in 2m 31s —— View job Todo list
ReviewWent through each self-audit item in the trigger comment independently rather than trusting the write-up. All of them hold up. 1. FQN-stability argument — correctly retired. 2. SpotBugs claim — the correction is real, not cosmetic. Checked 3. Deferral inversion — sound reasoning, correctly left queued rather than done. The logic in 4. Copyright control-arm claim — verified by reading the checker's logic, since I couldn't execute the script in this run. The 5. #40 section — cleanly deleted, not left stale. 6. Test-jar re-verification — confirmed on the current tree, not just asserted: One thing not in the trigger's list, worth a note rather than a blocker: the code content of No issues found. This is a well-executed re-cut: it distinguishes cleanly between "argument that quietly stopped being true" (FQN stability, SpotBugs visibility) and "argument that's still true" (nothing-internal-becomes-public, test-jar dependency unchanged), and every corrected claim is now backed by something checkable rather than asserted from memory. |
… flags `repo: hygiene` went red on this branch without a line of it changing: master gained `bin/check-branch-self-reference.sh`, and the re-cut renamed this PR's note from `pr-159-longpolling-to-main.md` to `pr-202-longpolling-to-main.md` and retitled its mentions to #202. The gate's PR-number arm reads whole files, so six mentions of #202 became unattested self-references and the job failed. The two mentions are different cases and get different markers, which is the distinction the gate's header insists on: - `docs/inflight/pr-202-longpolling-to-main.md` is this PR's own note - the gate names exactly that case for `post-merge: exempt-file`, a scope opt-out rather than an attestation. The merge deletes the file, so no sentence in it survives to read wrongly. - `docs/inflight/branch-package-rename-sweep.md` cites #202 twice, both as past-tense record of what the package-rename sweep found ("verified on #202"; "#202 traced the whole cascade back to one deleted line"). Those read identically after the merge - PRs are permanent, and it is the branch and the present tense that rot - so they take `post-merge: checked` blocks. Both are prose/table spans, so the block form avoids splitting a paragraph or a table row with an inline comment. Verified with `GITHUB_HEAD_REF=refactor/159-longpolling-to-main PR_NUMBER=202 bin/check-all.sh --with-tests`: 41 ran, 41 passed, previously 40 passed / 1 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMeraeBL2ycXVHEaTHM86W
astubbs
changed the title
…onto master
The note carried `post-merge: exempt-file - the merge deletes it`, and said so again
in its opening paragraph. docs/inflight/AGENTS.md forbids exactly that:
Never leave a "delete this when #NN merges" marker on master. The merge is
exactly when nobody is looking here, so the marker outlives the work and the
next reader inherits a stale note that reads as live.
Merging the PR would have ADDED a file whose own text says the merge removes it -
so the marker would have shipped and then sat there. The directory's four outcomes
say to migrate what outlives the work first, and `git rm` only once nothing left is
both true and unowned.
Two of its three items were already owned by docs/refactoring.md and were being
restated rather than staged: the rehome out of `internal.utils` (with the reasoning
for doing it in 0.6.0.0 rather than after) and the NN_NAKED_NOTIFY false positive
with its measurement.
The third was not owned anywhere, so it is migrated here rather than lost: that this
move is a PREREQUISITE and not a fix for the test-jar, and that `KafkaTestUtils` -
not the Truth subject, which cannot move without promoting com.google.truth to
compile scope for every downstream user - is where anyone closing confluentinc#162 /
confluentinc#861 should start.
Nothing else cited the note; check-file-refs, check-inflight-tags and
check-issue-refs are clean after the removal.
…owners
The note carried `post-merge: exempt-file - ... it is deleted when that PR lands`.
docs/inflight/AGENTS.md forbids exactly that:
Never leave a "delete this when #NN merges" marker on master. The merge is
exactly when nobody is looking here, so the marker outlives the work and the
next reader inherits a stale note that reads as live.
Merging would have ADDED a file whose own text says the merge removes it, so the
marker ships and then sits there. Same defect #202 had, fixed the same way:
the directory's four outcomes say migrate what outlives the work first, and `git rm`
only once nothing left is both true and unowned.
MIGRATED, because none of it dies with the branch:
- The defect class - eight live instances, two named as worth doing next, and three
dismissed with reasons - is SPLIT into its own note,
docs/inflight/bug-unbounded-log-lines.md, rather than compressed into
docs/refactoring.md.
That file is explicitly for refactors "too small to deserve their own note, a line
or two each"; this carries anchors, a decision about where the fix belongs (on the
type, not the call site) and the dismissals, which are the part a line would lose.
RecordBatchSummary's javadoc now points at it, so the next person writing a batch
log line meets it from the code rather than by grepping.
- Every anchor in it was re-grepped rather than copied, and two claims were corrected
in the move. #168 (confluentinc#629) asked for exactly the topic/partition/
offset that ConsumerOffsetCommitter's error line carries, so a fix there must keep
the identifiers and cap only the metadata string - the note previously read as
though the whole map could be summarised away. And the examples module is not the
"single WorkContainer, one key no value" case the three library modules are: CoreApp
interpolates a raw ConsumerRecord, whose Kafka toString does print the value. It
stays dismissed (one record, bounded, user-editable example code), but for a
different reason than it was recorded under.
- The two inline `ListAppender` copies still in SubmitWorkToPoolShutdownRaceTest, with
the standing follow-up to treat LogCapture as the only way to do this, go to
docs/refactoring.md beside the other copy-pasted-test-helper entries. That one IS a
line or two, so it belongs there and not in a note.
- The branch-specific flake lead moves into UserFunctionFailureLoggingTest's own
javadoc, next to the @isolated it explains: without that annotation,
closeAfterSingleMessageShouldBeEventBasedFast and
queuedMessagesNotProcessedOrCommittedIfSubmittedDuringShutdown failed. Naming them
there makes the lead reachable by grepping either test's name, which is how a future
debugger of those two would arrive - a deleted branch note is not.
CONFIRMED ALREADY OWNED, so it dies with the note rather than being restated:
- The fork<->upstream mapping - the upstream-pr-log-noise entry in
src/docs/development/upstream-map.yaml carries it, including why confluentinc#918/confluentinc#919 were read
and not taken, and that the entry stays open after this lands.
- LogCapture's two hazards - its class javadoc states both, and which fix each takes.
- The log.debug inside logWithoutEscaping - the comment beside it, written in 394f462.
- The #201 overlap - settled: origin/fix/155-load-factor-noise has merged this
branch and LoadFactorCeilingReportingTest now uses the shared LogCapture, so there is
nothing left to coordinate in either merge order.
- "AbstractParallelEoSStreamProcessor.java is a contended file" - transient by nature,
and the note itself said the answer is `gh pr diff --name-only`, not a file.
Nothing in the tree cited the deleted note: `grep -rn pr-log-verbosity` is empty, and
check-all.sh (15 gates, including check-file-refs, check-inflight-tags,
check-branch-self-reference and check-issue-refs) plus `bin/todo-index.sh --check` are
clean after the removal. Core unit suite unchanged at 546 run / 0 failures / 0 errors
/ 8 skipped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CMeraeBL2ycXVHEaTHM86W
🧪🔒 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 |
🟢 Throughput — OKThis branch measured about 2% 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 5 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. Updated for |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #202 +/- ##
============================================
- Coverage 81.45% 81.11% -0.35%
- Complexity 0 1393 +1393
============================================
Files 83 91 +8
Lines 4448 4934 +486
Branches 456 478 +22
============================================
+ Hits 3623 4002 +379
- Misses 644 737 +93
- Partials 181 195 +14
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:
|
…tor-noise Second take of #203's branch, now at a615560, which brings origin/master through 88f5774 with it: #202 (LongPollingMockConsumer ships in the main artefact), #415 (the quarantine lane retracts its last report) and #416 (a hook harness that scored silence as a pass). #202 is the one worth naming: it moves LongPollingMockConsumer from the test tree into parallel-consumer-core's main artefact, so this merge changes what is on the main compile path rather than only what is on the test one. Verified accordingly, not assumed - merge-tree predicted a clean tree (d3da954) and the merge produced no conflicts, but a clean auto-merge is exactly what compiled to nothing on this branch once before. Merge, never rebase: #201 is the child of #203 and both carry live review history. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMeraeBL2ycXVHEaTHM86W
Description
Moves
LongPollingMockConsumerfromparallel-consumer-core/src/test/javatosrc/main/java, sodownstream projects can use it with a plain dependency on
parallel-consumer-coreinstead of a<classifier>tests</classifier>one.Closes #159, the
upstream-mirrorissue for confluentinc#526.The AK
MockConsumerreturns frompoll()instantly, so a control loop under test spins instead oflong-polling.
LongPollingMockConsumerfixes that, and it is the only realistic way to test aparallel-consumer integration without a broker - but until now the only way to get it was a test-jar
dependency, which is not transitive, resolves awkwardly
(confluentinc#162) and drags the whole
test surface along.
This branch was re-cut onto today's master, discarding its own copy of the
io.confluent.*→bz.stub.*package rename now that master has completed that rename itself. Two of the argumentsthis PR was originally built on did not survive that; both are corrected below rather than quietly
dropped.
The package moved anyway - and the old rationale for keeping it is retired
The class now lives at
bz.stub.parallelconsumer.internal.utils.LongPollingMockConsumer. Only thesource root changes in this PR; the package name changed independently, on master, for the whole
fork.
The original version of this PR argued that keeping the fully-qualified name made the downstream
migration "a pom edit with no code change". That argument is dead. The fork-wide rename broke
every downstream import regardless of what this PR does, so FQN stability is no longer something
this change can offer or protect.
What survives is the argument that actually mattered:
artefact -
Range,JavaUtils,TimeUtils,LoopingResumingIteratorand more are public in themain jar today. This move does not promote an internal-only package to published API; that
happened long ago.
The deferral has inverted, and this is a decision worth taking deliberately. A supported
test-support class does not belong in a utils grab-bag - it wants
bz.stub.parallelconsumer.testing,or its own
parallel-consumer-test-supportmodule in thekafka-streams-test-utilsmould. Thatrehome was previously parked behind the confluentinc#271 package restructure, to avoid migrating the
same users twice. But
docs/refactoring.md's breaking-change section now opens with "The gate iscurrently OPEN: 0.6.0.0 is that major, it is unreleased", and 0.6.0.0 already carries the rename as
a break. So doing the rehome in 0.6.0.0 is one migration, and deferring it past 0.6.0.0 is now
what costs a second one. That
internalin the current package name is itself an argument formoving it. Left queued rather than taken here, but it is now a live call rather than a settled one.
Correction: the SpotBugs finding was already reported, rename or no rename
The original version of this PR said the
NN_NAKED_NOTIFYfinding on this class was newly visiblerather than newly introduced, because "SpotBugs analyses main sources" and the class was moving into
them. That was false. Master's root POM now sets
includeTestsonspotbugs-maven-plugin.Measured on master before this change, with the class still in
src/test:It is already reported today. This move changes the source root, not whether the class is analysed.
The finding remains a false positive and remains deliberately unfixed, so this PR stays a pure
relocation; the diagnosis and the deferred fix are recorded in
docs/refactoring.mdalong with thatreproduce line.
This does NOT eliminate the test-jar dependency - re-verified on today's master
confluentinc#162 and confluentinc#861 are cited on #159 as consequences of the test-jar. This
move is a prerequisite, not a fix, and that is unchanged:
KafkaTestUtils,AbstractParallelEoSStreamProcessorTestBaseandLongPollingMockConsumerSubjectare all still under
src/test- confirmed on today's master.<classifier>tests</classifier>(mutiny, vertx, reactor, and fourexample modules plus example-metrics), and their test sources still reference those classes.
LongPollingMockConsumerSubject: it is Truth-generator glue extendingcom.google.common.truth.Subject, so moving it would promotecom.google.truthfromtesttocompilefor every downstream user - exactly the cost this change was careful not to incur.The README's "
mvn compilefails, usemvn test-compile" note therefore still stands and is leftalone. If someone wants to actually close confluentinc#162, the next question is
KafkaTestUtils-the next-largest reason the classifier exists, and unlike the Truth subject it has no test-scope-only
dependencies.
What it dragged with it: nothing
The class references
AbstractParallelEoSStreamProcessor(main),org.apache.kafka.clients.*,Lombok, SLF4J and
pl.tlinkowski.unij. No test-only class came with it.parallel-consumer-core/pom.xmlis untouched - all four dependencies are already
compilescope, so no downstream user gains a new one.The single test-only reference was a javadoc
@see KafkaTestUtils#assertCommitLists(...)on twomethods, which would have become a dangling link in the published javadoc (
-Xdoclint:nonemeans itwould have shipped broken rather than failed). Rewritten as plain text naming the class. Confirmed by
building the javadoc jar, which now publishes
LongPollingMockConsumer.htmlcleanly.The #40 collision is gone, not resolved
The in-flight note previously recorded a collision with #40 (dedupe the
MockConsumer*tests)as "resolved itself". #206 merged on 2026-08-14, so there is nothing left to track.
docs/inflight/AGENTS.mdsays a note tracks only currently-OPEN work and forbids rewriting closeditems into a DONE narrative, so that section was deleted rather than updated.
The note itself was renamed
docs/inflight/pr-159-longpolling-to-main.md→docs/inflight/pr-202-longpolling-to-main.md: the directory's rule is now<area>-<NNN>-<slug>.mdwhere
pr-takes a fork PR number, and 159 is an issue. It also gained the requiredinflight-type: task/inflight-impact: coordinationtags - that tagging scheme and its gate(
bin/check-inflight-tags.sh) did not exist when this branch was first cut.Copyright registration, verified load-bearing
The relocation is registered in
bin/check-copyright-headers.sh'sRENAMED_FROM_UPSTREAMtable, withthe new path in
bz/stubspelling and the old path keepingio/confluent, as that table's own commentrequires.
Verified with a control arm rather than assumed:
PACKAGE_MOVESmaps the package but not the sourceroot, so without this entry the file resolves to a
src/mainfork-point path upstream never had.Deleting the line takes the check from 0 → 1 violation, naming this file
(
FAIL (fork-original file claims Confluent copyright)). The header itself needed no edit - master'srename already added the modifications line.
No
upstream-map.yamlentryDropped as a no-op.
docs/upstream.mdstates the manifest maps upstream PRs; #159 is theupstream-mirrorissue for confluentinc#526, so it already holds that mapping.bin/check-upstream-map.shpasses with no entry.
Repo hygiene:
repo: hygienewent red without this branch changingMaster gained
bin/check-branch-self-reference.shafter this branch was first cut, and the re-cutrenamed the note to
pr-202-longpolling-to-main.mdand retitled its mentions to #202. Thatgate's PR-number arm reads whole files, so six #202 mentions became unattested
self-references and the job failed. Two different cases, so two different markers:
docs/inflight/pr-202-longpolling-to-main.mdtakespost-merge: exempt-file- the scope opt-outthe gate's own header names for exactly this case, a PR note named for the PR it is about. The
merge deletes the file, so no sentence in it survives to read wrongly.
docs/inflight/branch-package-rename-sweep.md- master's file, otherwise untouched here - citesrefactor(core) astubbs#159: ship LongPollingMockConsumer in the main artefact #202 twice, both as past-tense record of what the package-rename sweep found ("verified on
refactor(core) astubbs#159: ship LongPollingMockConsumer in the main artefact #202"; "refactor(core) astubbs#159: ship LongPollingMockConsumer in the main artefact #202 traced the whole cascade back to one deleted line"). Those read
identically after the merge - PRs are permanent, and it is the branch and the present tense that
rot - so they take
post-merge: checkedblocks. Block form rather than inline, so the markersplits neither a paragraph nor a table row.
Verification
./mvnw -pl :parallel-consumer-core -am test→Tests run: 533, Failures: 0, Errors: 0, Skipped: 8, BUILD SUCCESSparallel-consumer-core-0.6.0.0-SNAPSHOT.jarcontainsbz/stub/parallelconsumer/internal/utils/LongPollingMockConsumer.class✔...-tests.jarcontains onlyLongPollingMockConsumerSubject.class- the Truth subject that stays ✔...-javadoc.jarnow publishesLongPollingMockConsumer.html; javadoc builds clean ✔bin/check-copyright-headers.sh→ 489 files, 0 violationsbin/check-issue-refs.sh→ no unqualified references below #1000 on added linesbin/check-inflight-tags.sh,bin/check-file-refs.sh,bin/check-branch-self-reference.sh,bin/check-upstream-map.sh,bin/check-docs-data.sh,bin/todo-index.sh --check→ all cleanGITHUB_HEAD_REF=refactor/159-longpolling-to-main PR_NUMBER=202 bin/check-all.sh --with-tests→ 41 ran, 41 passed (was 40 passed / 1 failed on
check-branch-self-reference.sh)Checklist
docs/inflight/pr-202-longpolling-to-main.md(new, replacing thepr-159-name),docs/refactoring.md(breaking-change queue + the corrected SpotBugs item),docs/inflight/branch-package-rename-sweep.md(twopost-merge: checkedattestations, seeabove), and no
upstream-map.yamlchange - see above.CHANGELOG.adocdeliberatelyuntouched, per AGENTS.md.
exercised by the whole core unit suite through
AbstractParallelEoSStreamProcessorTestBase.What this change actually needs verifying is that the artefacts come out right, which the jar
content checks above cover.
arguments are explicitly retired above rather than left standing.
executes.
bin/check-copyright-headers.shis run by.github/workflows/copyright.yml. The changeadds one
newpath|oldpathline to theRENAMED_FROM_UPSTREAMregistry so the scanner keepstreating the relocated file as upstream-derived. No scanner logic, no workflow definition and no
runner configuration changed; the registry is inert data the check reads, it grants no permission,
and the job runs on the hosted runner exactly as before.
Refs #159 · confluentinc#526 confluentinc#526