Repository navigation
refactor: Remove static state - #405
Closed
Antony Stubbs (astubbs) wants to merge 187 commits into
Closed
Antony Stubbs (astubbs) wants to merge 187 commits into
Antony Stubbs (astubbs) wants to merge 187 commits into
Conversation
… available by expanding locking scope Don't finish producing with lock until work as been put into work inbox so that controller can process it before arranging commits. Aquire the commit lock earlier, so that no more records are able to be sent between process results and committing transaction. Otherwise a record from a future commit coule be added to this transaction, without its source offsets being collected for commit due to the work's status not being completed (as it hasn't been processed by the controller yet).
…-static # Conflicts: # parallel-consumer-core/src/main/java/io/confluent/parallelconsumer/state/PartitionState.java # parallel-consumer-core/src/test/java/io/confluent/parallelconsumer/ParallelEoSStreamProcessorTest.java
# Conflicts: # parallel-consumer-core/src/main/java/io/confluent/parallelconsumer/offsets/OffsetSimultaneousEncoder.java # parallel-consumer-core/src/main/java/io/confluent/parallelconsumer/state/PartitionState.java
Antony Stubbs (astubbs)
changed the title
Antony Stubbs (astubbs)
left a comment
Antony Stubbs (astubbs)
left a comment
Contributor
Author
There was a problem hiding this comment.
Identified a couple of issues
…-static # Conflicts: # parallel-consumer-core/src/test/java/io/confluent/parallelconsumer/AbstractParallelEoSStreamProcessorTestBase.java # parallel-consumer-core/src/test/java/io/confluent/parallelconsumer/JStreamParallelEoSStreamProcessorTest.java # parallel-consumer-core/src/test/java/io/confluent/parallelconsumer/ParallelEoSStreamProcessorTestBase.java
Antony Stubbs (astubbs)
left a comment
Antony Stubbs (astubbs)
left a comment
Contributor
Author
There was a problem hiding this comment.
lgtm, get tests passing
Antony Stubbs (astubbs)
left a comment
Antony Stubbs (astubbs)
left a comment
Contributor
Author
There was a problem hiding this comment.
lgtm!
Contributor
|
Closing - Stale. |
Antony Stubbs (astubbs)
referenced
this pull request
in astubbs/parallel-consumer
Jul 28, 2026
* docs: add refactoring backlog (deferred internal refactors) A versioned, grouped-by-file list of refactors too big/risky to fold into the change at hand - captured for quiet time instead of GitHub issues (overkill for a solo maintainer, and keeps the fork tracker signal). Cross-cutting/architectural items (thread model / poll+control merge, God-class split, static-state removal, actor-IPC) link to upstream #200/#488/#524/#325/#405 and the abandoned draft branches as design refs rather than being rewritten. Seeded from a code scan (TODO/FIXME + large-class signals) and a branch/issue/prior-PR sweep. AGENTS.md documents the // TODO(refactor): marker convention, the doc boundaries (vs inflight / upstream-map / solutions / PR review), and the graduation rule. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(refactoring): enumerate abandoned branches + prior drafts as a real index Stop brushing over the pointers. Each abandoned refactor branch now has a specific one-liner (what it did, relevance, linked issue/PR) grouped by theme: thread-model/actor cluster (upstream #200), static-state removal, shard-count caching perf (confluentinc#530), engine/queue experiments (confluentinc#884), encoding, offsets/state classes (#233), API/interface, test infra. Records dead-ends explicitly (e.g. producer-facade, whose branch concluded it was not worthwhile) and supersessions (loom -> upstream confluentinc#908). The bulk verdicts for the ~53 prior closed PRs stay in upstream-pr-analysis.adoc; this index keeps the actionable pointers with issue links. Also added the two other synchronized(this) lock-hygiene sites (ProducerManager.syncBeginTransaction, DynamicLoadFactor.doStep) surfaced while checking the PR #57 fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(refactoring): address PR #67 review - Add a dedicated "Breaking changes queued for next major version" section (release-gated, kept apart from the non-breaking refactors), consolidating the deprecated commitInterval/options removals, JStream removal, enum rename, and nonnull/module-info candidates. Slim the by-file entries to back-references. - Pin every abandoned origin/<branch> reference to its current short SHA so the catalogue survives future branch cleanup; expand the brace-compressed lists. - Disambiguate bare upstream refs that collide with fork branch names (confluentinc#857/confluentinc#859/confluentinc#908 -> upstream #NN) and note the convention that branch names encode the upstream number. - Explain why so many draft branches exist: the fork was a personal experiment space before becoming the project's primary; mine them for ideas, then prune. - AGENTS.md: mention the new release-gated breaking-changes section. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Antony Stubbs (astubbs)
referenced
this pull request
in astubbs/parallel-consumer
Aug 1, 2026
… list Correcting this PR's original shape. It shipped a docs/TODO_TRIAGE.md that duplicated docs/refactoring.md - a 295-line backlog that already had a release-gated 'Breaking changes queued for next major version' section, a 'Remove static state (unblocks parallel test execution)' section, and per-file entries for #233, the encoders and WorkContainer. My triage re-derived all of that from scratch, with worse provenance: refactoring.md carries the upstream issue numbers (#405, #126, #143) and design branches that I did not have. The mistake was concluding 'no breaking-changes list exists' after grepping only docs/inflight.md. Too narrow a search to support the conclusion drawn from it. So: - delete docs/TODO_TRIAGE.md rather than maintain two taxonomies; - fold into refactoring.md the two things it genuinely did NOT cover: the loop-cycle waits in ParallelEoSStreamProcessorTest (a known intermittent failure, where cycle-counting IS the flake mechanism), and the MockConsumer.groupMetadata() workaround duplicated across four example tests; - point TODO_INDEX.md and docs/inflight.md at refactoring.md as the place triage happens, with an explicit 'do not start a parallel list'. docs/TODO_INDEX.md still earns its place: refactoring.md's capture convention is TODO(refactor):, of which there are 3, while the index inventories all 91 plain markers - a population nothing was tracking. AGENTS.md now says refactoring.md owns marker triage, and to read it before concluding some category of deferred work is untracked, since it is easy to miss by grepping only inflight.md. That note is the actual fix for the mistake. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
Antony Stubbs (astubbs)
referenced
this pull request
in astubbs/parallel-consumer
Aug 3, 2026
…actoring backlog (#103) 91 TODO/FIXME/XXX markers were scattered across the tree with nothing tracking them. This adds a generated inventory, and triages the useful ones into the backlog that already exists rather than a new one. docs/TODO_INDEX.md - generated inventory bin/todo-index.sh walks the tracked tree and writes every marker, grouped by module with file and line. --check exits non-zero when the committed index is stale, so it can be wired into CI later. This does not overlap docs/refactoring.md's existing // TODO(refactor): convention - that captures 3 markers; this inventories all 91, a population nothing was tracking. Triage goes in docs/refactoring.md An earlier revision of this PR added a separate docs/TODO_TRIAGE.md. That was wrong and has been removed. docs/refactoring.md was already a 295-line backlog with a release-gated 'Breaking changes queued for next major version' section (already listing the deprecated commitInterval options I had 'discovered'), a 'Remove static state' section carrying upstream refs (#405, #126, #143) and design branches I did not have, and per-file entries for #233, the encoders and WorkContainer. My triage re-derived that taxonomy from scratch with worse provenance. The root error: concluding 'no breaking-changes list exists' after grepping only docs/inflight.md. Too narrow a search to support the conclusion drawn from it. Two findings genuinely absent from refactoring.md were folded into it: the loop-cycle waits in ParallelEoSStreamProcessorTest (not a style point - that class is on the known intermittent-failure list, and cycle-counting IS the flake mechanism), and the MockConsumer .groupMetadata() workaround copy-pasted across four example tests (one Apache Kafka defect, four times; not ours to fix, but the duplication is). The guardrail is the durable part AGENTS.md gains a 'Where things live' table - one row per document, what it owns and what it is explicitly NOT for, since the boundaries are what get guessed wrong. There was no docs index at all: pointers were scattered across six sections, and docs/SELF_HOSTED_RUNNER.md was linked from nowhere. That layout only works if you already know which section to read, which is circular - and is how this PR went wrong. An earlier guardrail attempt lived inside the 'Refactoring backlog' section, so it only reached someone who had already found the file it was warning them about. The warning is now at the top, before the mistake rather than after it. Why .adoc/.md are not scanned Raised in review, and recorded as a comment in list_files() rather than left as an unexplained omission. It was tried: scanning docs added 4 hits and not one was a marker. Three were docs/refactoring.md QUOTING markers the index already lists from their .java source - so the same work appeared twice and the count inflated - and the fourth was prose. That is structural, not bad luck: refactoring.md is where marker triage lives, and AGENTS.md / inflight.md / CHANGELOG.adoc describe this tool, so scanning docs means indexing the index-of-work. The generated index is the reductio - alone it accounts for ~95 self-referential hits. The comment also says what to do instead: if a genuine marker lands in prose, move it into the code it concerns, where the scan will find it. Marker detection 101 on the first pass, 91 real. Excluded with reasons in the script: shell variables (todo=(), ${todo[*]}), YAML field keys (upstream-map .yaml uses todo: as a schema field), string literals, prose referencing a marker elsewhere, and compound names where the word is part of a filename (todo-index.sh). That last is live, not theoretical: merging master brought in #102's workflow comment naming the script and the scanner counted it as a 92nd marker until filtered - caught by --check, a fair self-validation of the tool. One self-inflicted bug found while building it: the anchored exclusions were applied to grep -n output, so ^ never matched and they silently did nothing. Noted in the script so it is not reintroduced. Not wired into CI as a gate. --check exists for it, but making it gating means every added or removed marker fails the build until regenerated - worth deciding deliberately, not as a side effect here. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
4 tasks done
Antony Stubbs (astubbs)
added a commit
to astubbs/parallel-consumer
that referenced
this pull request
Aug 14, 2026
A mirror records what a closed PR said; it does not keep the code. The 35 PRs closed in the 2023-06-15 sweep were reachable through refs/pull/<n>/head in the upstream repository - not a copy we control. If that repo is deleted, or a contributor's fork disappears, the commits go with it and the mirrors describe work whose diff no longer exists. Checked each of the 35 against this fork's refs. 29 were already safe, raised from branches that still exist here. Six were reachable only from upstream, and were not even present in a local clone: confluentinc#22, confluentinc#204, confluentinc#270, confluentinc#405, confluentinc#443 and confluentinc#506. All six are now pinned as annotated archive/upstream-pr-<n> tags carrying the upstream title, author, head branch and closure date, so provenance survives without the upstream thread. confluentinc#443 was the sharpest case - a third-party fork (Robbie-Palmer) can vanish independently of Confluent's repository and nothing would have flagged it. Tags rather than branches: they are not swept by branch-cleanup tooling and read as archival rather than live work. Recorded as a recurring check, since a head safe today can be orphaned tomorrow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 of 4 tasks
Antony Stubbs (astubbs)
added a commit
to astubbs/parallel-consumer
that referenced
this pull request
Aug 14, 2026
…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>
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
2 participants
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.
Removes static state and manipulation. Depends on DI module and transaction-docs.