Major refactor: Package restructure by astubbs · Pull Request #271 · confluentinc/parallel-consumer

Antony Stubbs (astubbs) · 2022-04-22T15:08:35Z

Description...

Checklist

Documentation (if applicable)
Changelog

- Removes onPartitionsLost handlers as we don't need to do anything differently than from onPartitionsRevoked

Antony Stubbs (astubbs) · 2022-04-22T17:33:36Z

+ * Copyright (C) 2020-2022 Confluent, Inc.
+ */
+
+public class Message {


Antony Stubbs (astubbs) · 2022-04-22T17:34:03Z

-        long highestSucceeded = partitionState.getOffsetHighestSucceeded();
+    byte[] encodeOffsetsCompressed(long finalOffsetForPartition, HighestOffsetAndIncompletes partitionState) throws NoEncodingPossibleException {
+        var incompleteOffsets = partitionState.getIncompleteOffsets();
+        //long highestSucceeded = partitionState.getOffsetHighestSucceeded();


Antony Stubbs (astubbs) · 2022-04-22T17:34:11Z

+import java.util.HashMap;
+import java.util.Map;
+
+@Value


Antony Stubbs (astubbs) · 2022-04-22T17:34:37Z

+ * Commit request message
+ */
+@Value
+public class CommitRequest {


Antony Stubbs (astubbs) · 2022-04-22T17:34:40Z

+ * Commit response message, linked to a {@link CommitRequest}
+ */
+@Value
+public class CommitResponse {


Antony Stubbs (astubbs) · 2022-04-22T17:34:52Z

+@RequiredArgsConstructor(access = PRIVATE)
+@Getter
+@EqualsAndHashCode
+public class ControllerEventMessage<K, V> {


Antony Stubbs (astubbs) · 2022-04-22T17:35:01Z

+import java.util.Collection;
+
+@Value
+public class PartitionEventMessage {


Antony Stubbs (astubbs) · 2022-04-22T17:36:14Z

                log.debug("Vert.x Vertical success");
                wc.onUserFunctionSuccess();
-                addToMailbox(wc);
+                super.bus.sendWorkResultEvent(wc);


implement delegate for add to mb

Edward Vaisman (eddyv) · 2023-06-15T14:37:04Z

Closing - Stale.

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

…able Review of this PR found the preservation work sound but the write-up wrong in two places, and unbacked in a third. Corrects the confluentinc#443 rationale, which was fabricated. Its head label is confluentinc:pyallel-consumer and that branch is still live upstream; the contributor's own fork is already gone and made no difference. All 35 swept heads carry confluentinc: labels, so the exposure is upstream branch or repository loss, not a contributor's fork vanishing. The original text inferred the risk from the PR author field without checking the head repo. Corrects "29 were already safe, because their PRs were raised from branches that still exist on this fork". Raised-from is not the criterion that was used or that holds: confluentinc#271's own branch is gone and its head survives only because an unrelated branch contains it, while confluentinc#22, confluentinc#270 and confluentinc#405 have same-named fork branches that do NOT contain their heads. The wording is now reachable-from, and the method records that stale remote-tracking refs were reconciled against a live ls-remote. Records the six SHAs in upstream-map.yaml under sweep-2023-admin-closure with the tag names and a re-verify command, so the containment check can be redone without re-querying upstream - PR numbers alone did not allow that. The cohort comment there said all heads "were verified reachable", which this work disproved for six of them; it now says what was actually found. Stops claiming a recurring check that nothing performs. --audit covers tracking and mirroring, not reachability, and would report clean with every archive tag deleted. Named as a manual step until a containment check is wired in. Also drops the fork-network durability implication (deleting a public parent re-roots the network rather than destroying objects, so the tags do discharge the stated threat), trims the refactoring entry to the file's one-line prior-art footnote style since fork-upstream mapping is explicitly out of scope there, and fixes which method holds the user-function try/catch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

… swept PR heads only upstream had (#301) Two records, both from reading back the #261 fix. Documentation only - the sole code change is a TODO marker. `InternalRuntimeException` misnames the produce-send failure ----------------------------------------------------------------------------- `ProducerManager`'s send callback throws `InternalRuntimeException` when a send fails in non-transactional mode, but that is an expected operational state, not an internal fault. The cost is rediscovery: a name saying "internal" sends every reader, human or agent, to re-derive where the callback runs, where its throw can actually escape, and what reaches it, before they can conclude this is ordinary failure handling. That derivation was done from source and kafka-clients bytecode during review and was recorded nowhere. The entry keeps two questions apart, because every loose version of this in review collapsed them. The callback is invoked on asynchronous failures too - `ProducerBatch` calls the same `onCompletion`, so the log fires for effectively every failed send. But a throw from it escapes to the caller only on `KafkaProducer#doSend`'s synchronous `catch (ApiException)` path; asynchronously, `ProducerBatch` catches and logs it. Only the second question governs the exception type, since only there is the thrown type observable to a caller. That set is narrower than "any pre-accumulator failure" - a serializer throwing `SerializationException` propagates without invoking the callback at all - and wider than "oversized records": `RecordTooLargeException` is the case `TransactionalPartialResultSetIT` exercises, while metadata and authorization failures reach the same handler. A refactor that instead models "the callback only runs synchronously" would drop logging for every asynchronous send failure. The recorded fix is non-breaking - a specific subclass, so existing `catch (InternalRuntimeException)` keeps working - with the caveat that makes it actually work: `processAndProduceResults` re-wraps into a generic `InternalRuntimeException`, so the subtype has to be preserved through that outer wrapper or callers still cannot catch it and the refactor buys nothing but a better log line. The lambda parameter should become `sendFailure`; the entry warns against naming it after user code, which is the confusion it exists to stop. A `TODO(refactor)` marker now sits at the site, since a backlog entry with no code-local marker is invisible to grep. Its summary stays on one physical line because `bin/todo-index.sh` indexes only that line - the first attempt wrapped, and generated the fragment "expected state, not an" with diagnosis and remedy dropped. This also corrects the in-flight note #261 added. The retryable half of terminal-vs-retryable exceptions *was* built: `PCRetriableException` is public and `AbstractParallelEoSStreamProcessor` honours it. What is missing is the terminal half - nothing can say "stop, this will never succeed". `confluentinc#291` is a PR, not an issue, implementing `confluentinc#242`, and both concern exceptions the user function throws rather than send-failure classification. Six swept PR heads existed only upstream - now preserved ----------------------------------------------------------------------------- A mirror records what a closed PR said; it does not keep the code. The 35 PRs closed in the 2023-06-15 administrative sweep were reachable through `refs/pull/<n>/head` in the upstream repository, which is not a copy we control. Two things turned out **not** to be loss events, and both were assumed to be in earlier drafts. Deleting the branch behind a PR does not lose the commit, since the base repository holds the pull ref. Nor does a contributor's fork vanishing: every one of the 35 heads was raised from a `confluentinc:` branch, so no third party holds any of them - including `confluentinc#443`, whose author's fork is already gone with no effect. The single upstream exposure is loss of `confluentinc/parallel-consumer` itself. The containment check is restricted to `origin/*`, and that restriction is the whole point: a full clone of this fork also carries `upstream`, and a head contained only in `upstream/*` is exactly the case being hunted. A bare `git branch -r --contains` searches every remote and would have reported every head preserved, so nothing would have been tagged. Reconciled against a live `git ls-remote --heads origin`, because stale tracking refs would otherwise count as safe. That split the cohort: 29 heads are contained by some branch still on this fork - contained by, not raised from; `confluentinc#271`'s own branch is long gone - and six are contained by nothing here. Those six are pinned as annotated `archive/upstream-pr-<n>` tags, whose messages carry the upstream title, author, head branch and closure date so provenance survives without the thread. Tags rather than branches: they are not swept by branch-cleanup tooling, and an annotated tag is fetched by every clone, which a pull ref is not. The tag names, target SHAs and check date live only in `sweep-2023-admin-closure.preserved_heads` in `upstream-map.yaml`. They are deliberately not duplicated into prose - a corrected SHA updated in one copy while the other still read as authoritative is the drift the record exists to prevent. What this does not do ----------------------------------------------------------------------------- The recurring check is **not** automated. `--audit` covers tracking and mirroring, not reachability, and would report clean with every archive tag deleted. It is a manual step until a containment check is wired into `upstream-sweep.sh`. There is also a second, fork-side risk this cannot remove: those 29 heads are safe only because an `origin/*` branch contains them, so deleting one of those branches re-orphans a head that reads as preserved today. That, not any upstream branch, is what a re-run has to watch. #300 tracks this alongside the rest of the upstream-closure decisions nobody has made yet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@see

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

#202) LongPollingMockConsumer moves from parallel-consumer-core's test sources into the main artefact, so a downstream project can depend on it with a plain parallel-consumer-core dependency instead of a <classifier>tests</classifier> one. Closes #159, the mirror of confluentinc#526. Why it matters: the Apache Kafka MockConsumer returns from poll() instantly, so a control loop under test spins rather than long-polls. LongPollingMockConsumer is the only realistic way to test a parallel-consumer integration without a broker, and until now the only way to get it was a test-jar dependency - non-transitive, awkward to resolve (confluentinc#162), and dragging the whole test surface along with it. Only the source root changes. The package name moved to bz.stub.parallelconsumer.internal.utils on master, for the whole fork, independently of this change. Nothing internal-only becomes public: that package already ships in the main jar (Range, JavaUtils, TimeUtils, LoopingResumingIterator). Two arguments the original version of this change rested on are retired rather than carried: that keeping the fully-qualified name made downstream migration a pom-only edit (the fork-wide rename broke every downstream import regardless), and that the NN_NAKED_NOTIFY SpotBugs finding on this class was newly visible because it entered main sources (false - includeTests is on, and the finding was already reported with the class in src/test; measured on master before this change). Decision left open, and now live rather than settled: 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-support module. That rehome was parked behind the confluentinc#271 restructure to avoid migrating users twice; with 0.6.0.0 unreleased and already carrying the rename as a break, doing it in 0.6.0.0 is one migration and deferring past it is what costs two. Recorded in docs/refactoring.md, alongside the finding that KafkaTestUtils is the next thing standing between downstream users and the test-jar - and that its Truth subject cannot follow the same route, because moving it would promote com.google.truth from test to compile scope for every user. Upstream-Issue: confluentinc#526 Forwarded: not-needed Applied-Upstream: no Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMeraeBL2ycXVHEaTHM86W

Antony Stubbs (astubbs) added 6 commits April 22, 2022 15:19

START: Shared nothing architecture - last step realise partition events

8a42ca6

- Removes onPartitionsLost handlers as we don't need to do anything differently than from onPartitionsRevoked

START: Refactor - move from type packages to area packages

52f4531

broke: tighten access

83e4ba5

broken: pass commit data through messages, instead of lookup

097f562

save

bd79643

save

d86a87e

Antony Stubbs (astubbs) commented Apr 22, 2022

View reviewed changes

This was referenced May 6, 2022

Release train for 1.0 #172

Open

major: #200 Shared nothing architecture - Partition Events #270

Closed

Antony Stubbs (astubbs) added this to the 0.6 milestone May 12, 2022

Antony Stubbs (astubbs) mentioned this pull request Sep 29, 2022

Consumer closes with an exception NoSuchElementException #419

Closed

Antony Stubbs (astubbs) changed the title ~~Improvements/package restructure~~ Major refactor: Package restructure Dec 7, 2022

Edward Vaisman (eddyv) closed this Jun 15, 2023

This was referenced Aug 5, 2026

confluentinc#172: Release train for 1.0 astubbs/parallel-consumer#135

Open

refactor(core) astubbs#159: ship LongPollingMockConsumer in the main artefact astubbs/parallel-consumer#202

Merged

Antony Stubbs (astubbs) mentioned this pull request Sep 8, 2026

docs(inflight): two notes for refactoring the God class and the five classes below it astubbs/parallel-consumer#479

Draft

2 of 6 tasks

Navigation Menu

Repository navigation

Major refactor: Package restructure - #271

Major refactor: Package restructure#271
Antony Stubbs (astubbs) wants to merge 6 commits into
masterfrom
improvements/package-restructure

Antony Stubbs (astubbs) commented Apr 22, 2022

Uh oh!

Antony Stubbs (astubbs) Apr 22, 2022

Uh oh!

Antony Stubbs (astubbs) Apr 22, 2022

Uh oh!

Antony Stubbs (astubbs) Apr 22, 2022

Uh oh!

Antony Stubbs (astubbs) Apr 22, 2022

Uh oh!

Antony Stubbs (astubbs) Apr 22, 2022

Uh oh!

Antony Stubbs (astubbs) Apr 22, 2022

Uh oh!

Antony Stubbs (astubbs) Apr 22, 2022

Uh oh!

Antony Stubbs (astubbs) Apr 22, 2022

Uh oh!

Edward Vaisman (eddyv) commented Jun 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

Antony Stubbs (astubbs) commented Apr 22, 2022

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Edward Vaisman (eddyv) commented Jun 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants