Transaction system improvements and docs by astubbs · Pull Request #355 · confluentinc/parallel-consumer

Antony Stubbs (astubbs) · 2022-07-15T13:25:10Z

Clarifies transaction system with much better documentation.
Fixes a potential race condition which could cause offset leaks between transactions.
Introduces lock acquisition time outs.
Fixes a potential issue with removing records from the retry incorrectly from having an inconsistency between compareTo and equals in the retry TreeMap.

Add bulk parallel transactions to features
change base branch to master
finish tests
document what happens upon timeouts - fail fast
changelog
consider cherry picking tangents

Blocked by:

Antony Stubbs (astubbs)

...

Antony Stubbs (astubbs)

...

Antony Stubbs (astubbs)

...

…retry queue due to inconsistency between equals and compareTo in TreeMap

Co-authored-by: Roman Kolesnev <88949424+rkolesnev@users.noreply.github.com>

Antony Stubbs (astubbs)

...

Roman Kolesnev (rkolesnev) · 2022-09-29T11:31:31Z

LGTM - already approved on previous review as suggestions were not fundamental.

…y it had been done

…oor Comparator implementation Further on for the fix from PR #355: "Fixes a potential issue with removing records from the retry incorrectly from having an inconsistency between compareTo and equals in the retry TreeMap."

…parator implementation (#423) Further on for the fix from PR #355: "Fixes a potential issue with removing records from the retry incorrectly from having an inconsistency between compareTo and equals in the retry TreeMap."

@Quarantined

…ach the gate about anchors Two things the reference sweep surfaced in Java sources. The quarantine script tests built their fixture registries with "Owner: PR #999", "PR #80" and "PR #123". The last two are real fork PRs, so the fixtures read as genuine references to anyone grepping, and #999 is close enough to the live range to be mistaken for one. They are now #999999 and #999998 - unmistakably fake, and above the threshold where a bare number is ambiguous. Two distinct values, because the owner-mismatch tests compare a @Quarantined value against a registry value and need them to differ. 27 tests pass. The gate also flagged references that were already qualified by an html anchor - TransactionMarkersTest has <a href=".../issues/329">Github issue #329</a>, where the href names the repo and the number in the link text is unambiguous to any reader. stripQualified removed bare URLs but not the anchor element, so the visible "#329" looked bare. It now strips the whole element, with a test confirming a bare number elsewhere on the same line is still caught. Together these take the tree-wide backlog from 390 to 374, and the Java share from 53 to 36. The follow-up note now carries the finished classification of all 36 Java references - file, line, and which repo each means, resolved against both - so that pass does not repeat this research. Also records that @tag("#355") is a tag-string rename rather than a prose prefix, and that nothing selects on that tag. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn

@tag

The reference gate requires any #NNN below the threshold to name its repo, because the fork's numbering sits entirely inside upstream's range and a bare number is a coin flip. It checks added lines only, so it never fired on text nobody was editing - leaving the convention true of the files the original work touched and false of the rest of the tree. Two passes, on the same lines and so landing together: - 366 bare references gain their repo. All 77 distinct numbers were resolved against BOTH repos first: 62 of them exist in each, meaning different things, so the classification is per-occurrence rather than per-number. #188 and #195 are fork mirror issues in the release notes while confluentinc#188 and confluentinc#195 are the upstream bugs cited in test comments in the same tree. - 24 `upstream #NNN` uses become the owner form. That form passed the gate but names a relationship rather than a repository, and this fork is itself upstream to anyone who forks it. Three sets are deliberately NOT prefixed, because they are not references: author ordinals ("run #1", "produce #1/#2", "NUDGE #1/#2") annotating log excerpts, reworded to plain numbers; the changelog gate's fixture, which asserts that a *bare* #NN is not a citation and would have been destroyed by qualifying it, so it moves above the threshold as a fake #999104; and upstream-pr-analysis.adoc, which is exempt and written entirely in upstream terms. README link text keeps its qualifier even though the URL beside it already names the repo - the gate can see a link target, a reader cannot, and this fork has its own #12. Quoted upstream titles keep the quotation intact with the number appended rather than having the owner inserted mid-title. README is generated: the edit is in src/docs/README_TEMPLATE.adoc. @tag("#355") becomes @tag("confluentinc#355"). Verified nothing selects on that tag - no pom, workflow or script filters it - and both classes still collect and pass. The 14 upstream-derived Java files gain the "Modifications Copyright" line the provenance-aware header check requires of any file changed since the fork point. docs/inflight/next-qualify-remaining-refs.md is deleted: this is everything it tracked, and in-flight files do not outlive their work. No behaviour change.

@tag

The reference gate requires any #NNN below the threshold to name its repo, because the fork's numbering sits entirely inside upstream's range and a bare number is a coin flip. It checks added lines only, so it never fired on text nobody was editing - leaving the convention true of the files the original work touched and false of the rest of the tree. Two passes, on the same lines and so landing together: - 366 bare references gain their repo. All 77 distinct numbers were resolved against BOTH repos first: 62 of them exist in each, meaning different things, so the classification is per-occurrence rather than per-number. #188 and #195 are fork mirror issues in the release notes while confluentinc#188 and confluentinc#195 are the upstream bugs cited in test comments in the same tree. - 24 `upstream #NNN` uses become the owner form. That form passed the gate but names a relationship rather than a repository, and this fork is itself upstream to anyone who forks it. Three sets are deliberately NOT prefixed, because they are not references: author ordinals ("run #1", "produce #1/#2", "NUDGE #1/#2") annotating log excerpts, reworded to plain numbers; the changelog gate's fixture, which asserts that a *bare* #NN is not a citation and would have been destroyed by qualifying it, so it moves above the threshold as a fake #999104; and upstream-pr-analysis.adoc, which is exempt and written entirely in upstream terms. README link text keeps its qualifier even though the URL beside it already names the repo - the gate can see a link target, a reader cannot, and this fork has its own #12. Quoted upstream titles keep the quotation intact with the number appended rather than having the owner inserted mid-title. README is generated: the edit is in src/docs/README_TEMPLATE.adoc. @tag("#355") becomes @tag("confluentinc#355"). Verified nothing selects on that tag - no pom, workflow or script filters it - and both classes still collect and pass. The 14 upstream-derived Java files gain the "Modifications Copyright" line the provenance-aware header check requires of any file changed since the fork point. docs/inflight/next-qualify-remaining-refs.md is deleted: this is everything it tracked, and in-flight files do not outlive their work. No behaviour change.

…e records that were wrong Ten open `upstream-mirror` issues had never been assessed against this tree. They turned out not to be untriaged: each carries a `## Fork status` section written when it was mirrored, and THAT SECTION - not upstream-map.yaml, not upstream-pr-analysis.adoc - is where the fork's only judgement about most upstream issues lives. Searching the two documents came up thin for exactly that reason. Most had drifted, and the errors were the load-bearing kind. #241 restates an account of `commitOffsets` that confluentinc#355 falsified in 2022. #163 says the poll path is unguarded when a typed per-exception seam already exists, so the fix is far cheaper than recorded. #161 says the scheduler moves the user function off the control thread; it was never on it, which changes the answer to the reporter's question. #173 contradicts our own README on what the transactional mode guarantees. #175 credits the wrong PR and omits the close-path failure the reporter called the bigger problem. #162 misses that reporters hit the same warning AFTER the fix shipped, which is the fact deciding whether it can close. WHY THEY GOT THAT WAY, which is the part that will recur and so is recorded at population level in `upstream-mirror-bodies-are-stale.md`: a tree-wide rename invalidated every `file:line` citation in every body in one commit, silently - one had been wrong since the day it was written, naming a method declaration rather than the statement it claimed. Nothing re-checks a body once written, unlike a map entry with its `last_checked`. And nothing in the format separates a verified claim from an inferred one, which is where every failure above sits. THREE VERDICTS IN upstream-pr-analysis.adoc ARE REFUTED, not softened, because each would have caused a wrong action. The worst: "merging confluentinc#893 + confluentinc#909 likely resolves confluentinc#777" - left standing, that closes confluentinc#777 the moment #57 merges, when upstream's own maintainer answered it in 2024 saying the redelivery is expected at-least-once behaviour. NOTE NAMES NOW CARRY THE FORK ISSUE NUMBER. A fifth of this directory already carried a number and the existing names disagree about what it means - `bug-857-family.md` is a confluentinc number, `perf-192-followups.md` a fork one. That is the coin flip docs/issue-references.md exists to prevent, with none of the protection, because a filename cannot be qualified. The three notes first added here as `upstream-543`, `upstream-777` and `upstream-809` were the sharpest case: all three numbers also exist as fork issues meaning other things. Older disagreeing names are left alone - renaming breaks every citation and improves nothing. Manifest corrections: `sweep-2023-tx-failure-taxonomy` restated the premise confluentinc#355 falsified, and `bug-833-commit-response-timeout` still read `pr-open` after #204 merged. No issue has been touched - every correction and reporter-facing answer is drafted in its note and left unposted, pending a maintainer's decision on each. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…e records that were wrong Ten open `upstream-mirror` issues had never been assessed against this tree. They turned out not to be untriaged: each carries a `## Fork status` section written when it was mirrored, and THAT SECTION - not upstream-map.yaml, not upstream-pr-analysis.adoc - is where the fork's only judgement about most upstream issues lives. Searching the two documents came up thin for exactly that reason. Most had drifted, and the errors were the load-bearing kind. #241 restates an account of `commitOffsets` that confluentinc#355 falsified in 2022. #163 calls the poll path unguarded when a typed per-exception seam already exists, so the fix is far cheaper than recorded. #161 says the scheduler moves the user function off the control thread; it was never on it, which changes the answer to the reporter's question. #173 contradicts our own README on what the transactional mode guarantees. #175 credits the wrong PR and omits the close-path failure the reporter called the bigger problem. #162 misses that reporters hit the same warning AFTER the fix shipped, which is the fact deciding whether it can close. WHY THEY GOT THAT WAY is recorded at population level in `upstream-mirror-bodies-are-stale.md`, because the causes are structural and will recur: a tree-wide rename invalidated every `file:line` citation in every body in one commit, silently - one had been wrong since the day it was written, naming a method declaration rather than the statement it claimed. Nothing re-checks a body once written, unlike a map entry with its `last_checked`. And nothing in the format separates a verified claim from an inferred one, which is where every failure above sits. THREE VERDICTS ARE REFUTED, not softened, because each would have caused a wrong action. The worst: "merging confluentinc#893 + confluentinc#909 likely resolves confluentinc#777". Left standing, that closes confluentinc#777 the moment the cherry-pick merges - when upstream's own maintainer answered it in 2024, before either PR existed, saying the redelivery is expected at-least-once behaviour. EVERY CLAIM HERE WAS RE-VERIFIED AGAINST HEAD, after a review found several that read as checked and were not. Two citation anchors returned nothing when grepped - one a fabricated quotation around a fair paraphrase, one a `grep` instruction naming a string absent from the file it pointed at. A `RetryQueue` assertion was TRUE WHEN WRITTEN and falsified hours later by this branch's own master merge, so it now records the defect as fixed and names where the stale wording survives. And the confluentinc#893 cherry-pick had already moved from #57 to #337 before these notes were written, which three places still got wrong - including the manifest entry, corrected here. NOTE NAMES NOW CARRY THE FORK ISSUE NUMBER. A fifth of this directory already carried one and the existing names disagree about what it means - `bug-857-family` is a confluentinc number, `perf-192-followups` a fork one. That is the coin flip docs/issue-references.md exists to prevent, with none of the protection, because a filename cannot be qualified. The three notes first added here as `upstream-543`, `upstream-777` and `upstream-809` were the sharpest case: all three numbers also exist as fork issues meaning other things. The Renovate note lands here rather than on its own, because it is a triage finding with the same shape as the rest: the case FOR switching did not survive opening `.github/dependabot.yml`, which already carries the scoped ignores and the grouping the argument asked for. Kept for its counter-evidence, so the idea meets it rather than being re-derived. No issue has been touched - every correction and reporter-facing answer is drafted in its note and left unposted, pending a maintainer's decision on each. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@tag

…g it ParallelConsumerOptions#validate() now throws IllegalArgumentException when initialLoadFactor is above maximumLoadFactor, naming both options and both values so the user sees the transposition without reading library source: Cannot set initialLoadFactor (100) above maximumLoadFactor (9) - the initial load factor is where the dynamic load factor starts and the maximum is the ceiling it may step up to, so an inverted pair can never step Why this rides with the logging fix rather than standing alone. The commit before this one decides that DynamicLoadFactor classifies a factor as deliberately fixed on initial == maximum, deliberately == and not >=, because an inverted pair also cannot step and treating it as fixed would demote its ceiling report to debug - silencing the only signal a user with the typo would ever get. That reasoning only holds while the inverted pair is the noisier of the two, so the == is load-bearing on the absence of a check, and its javadoc pointed at docs/refactoring.md to say so. Landing the check in a later PR would leave a released version whose quietening rule is justified by a follow-up that has not happened. Landing both together lets the javadoc state the settled position: the pair is rejected at configuration, and the == still holds on its own because the constructor is internal and takes the two bounds directly. What a user saw before this. The pair was accepted and the factor pinned at the initial value, surfacing at best as an inverted "100/10" inside the rate-limited saturation warning - which fires only when the pool queue is below target, and reads as a capacity signal rather than as the misconfiguration it is. Checked whether or not messageBufferSize is set, even though a buffer size makes the pair unused: the alternative accepts a nonsensical value and lets it survive to the configuration change that starts reading it again. Breaking, and recorded as such. An application carrying the typo starts and runs today and fails at construction after this. Small blast radius, but "started yesterday, will not start today" is what a Breaking release note exists for, so it is written into the release-gated section of docs/refactoring.md, whose gate is open (0.6.0.0 is unreleased). The follow-up entry that section's sibling held for this work is removed in the same commit - the work is done, so the record of it being deferred goes. Same-defect-class sweep - "a paired-bounds option where only one ordering is meaningful, unvalidated". initialLoadFactor/maximumLoadFactor is the only instance in main code across every module: a sweep for min/max, initial/max and target/limit field pairs returns maxConcurrency, maxFailureHistory and DynamicLoadFactor#maxFactor, each a lone bound with no partner to be ordered against. Dismissed as a different class: messageBufferSize over the load factor pair (an override, not an ordering); retryDelayProvider over defaultMessageRetryDelay (documented override, already null-guarded at the call site with a fallback warn); maxConcurrency with batchSize (a product, not bounds); shutdownTimeout with drainTimeout (disjoint shutdown modes, neither bounds the other). batchSize is genuinely unvalidated but is a single-value lower bound, not a paired ordering, and is owned by #311 and docs/inflight/bug-unvalidated-batchsize.md. Test: ParallelConsumerOptionsTest gains the three cases - inverted rejected with both names and both values in the message, equal accepted (the fixed factor messageBufferSize produces, which this must not break), ascending accepted. Proved red by removing the validate() call: 4 run, 1 failure, and only invertedLoadFactorPairIsRejected, so the two accepting cases are not merely asserting the check exists. Restored, 541/541 green (538 baseline plus 3), skipped 8 unchanged; bin/check-all.sh 15 ran, 15 passed. The class-level @tag("transactions")/@tag("confluentinc#355") move onto setTimeBetweenCommits, which is what they describe - they were on the class when it held only that test, and left there a -Dexcluded.groups=transactions run would silently drop the new validation tests.

Antony Stubbs (astubbs) requested a review from Roman Kolesnev (rkolesnev) July 15, 2022 13:25

Antony Stubbs (astubbs) commented Jul 15, 2022

View reviewed changes

Comment thread src/docs/README_TEMPLATE.adoc

Antony Stubbs (astubbs) force-pushed the improvements/transaction-docs branch from 1e583cb to 5224e82 Compare July 15, 2022 22:04

Antony Stubbs (astubbs) mentioned this pull request Jul 15, 2022

fixes #29: Faster record producing - removes blocks on Future and processes results asynchronously #356

Closed

5 tasks

Antony Stubbs (astubbs) commented Jul 16, 2022

View reviewed changes

Antony Stubbs (astubbs) force-pushed the master branch from c097a37 to 3e0c5fd Compare August 2, 2022 11:07

Antony Stubbs (astubbs) added 11 commits August 18, 2022 13:14

START: Transactional mode docs

1bf346a

asciidoc

e61863d

docs

666375b

skeleton transaction producer blocking test

fb66c49

skeleton transaction producer blocking test

2c26cb5

step

c85d543

step - idea to not block tx system on get - depends on actor system

5449a95

step - batch the read lock holding

d24e7c9

review

b1b56e1

typo

c35de57

delete idea - saved in branch improvements/transactions-dont-block

0d99c2c

Antony Stubbs (astubbs) force-pushed the improvements/transaction-docs branch from 978ed08 to 0d99c2c Compare August 18, 2022 12:32

Antony Stubbs (astubbs) added 5 commits August 18, 2022 13:51

review

1aabc92

move/deprecate set time between commits to options

51e8be0

step - compile

845c066

review

af939c3

inline method for review ease

c5300e4

Antony Stubbs (astubbs) commented Aug 18, 2022

View reviewed changes

Comment thread ...l-consumer-core/src/test/java/io/confluent/parallelconsumer/ParallelConsumerOptionsTest.java

Antony Stubbs (astubbs) added 5 commits August 18, 2022 15:20

review

e7ae2b3

expand timed block and review

b19b19b

compiles.

10047ac

add test tags

24b7260

sendingGetsLockedInTx works

f7abb70

Antony Stubbs (astubbs) and others added 7 commits September 28, 2022 16:57

step: commitTimeout test with happy path tx

2184690

TANGENT: BUG: Fixed an issue where a WC may not get removed from the …

2b1e0fb

…retry queue due to inconsistency between equals and compareTo in TreeMap

step: Finish produceTimeout test

7474303

Apply suggestions from code review

b13df5d

Co-authored-by: Roman Kolesnev <88949424+rkolesnev@users.noreply.github.com>

license

49f4423

review

d098ce9

review

99dc46d

Antony Stubbs (astubbs) commented Sep 29, 2022

View reviewed changes

Antony Stubbs (astubbs) added 2 commits September 29, 2022 11:44

review

624e2a1

review

b596e03

Roman Kolesnev (rkolesnev) reviewed Sep 29, 2022

View reviewed changes

Comment thread ...llel-consumer-core/src/main/java/io/confluent/parallelconsumer/internal/ProducerManager.java Outdated

review

0fa556c

Antony Stubbs (astubbs) added 3 commits September 29, 2022 12:45

review - fix test - bad assertion - at most vs at least

8f38d30

docs

c97d30d

turn off forced transaction commit mode for vertx test - can't see wh…

e6c88d1

…y it had been done

Antony Stubbs (astubbs) merged commit 9423763 into confluentinc:master Sep 29, 2022

Antony Stubbs (astubbs) deleted the improvements/transaction-docs branch September 29, 2022 12:26

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

fixes ShardManager retryQueue ordering and set issues due to poor Comparator implementation #423

Merged

1 task done

Antony Stubbs (astubbs) mentioned this pull request Aug 20, 2026

docs(inflight): triage ten untriaged upstream mirrors, give the fork's branches one validated accounting, and index every issue so grep reaches the tracker astubbs/parallel-consumer#327

Merged

4 tasks done

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

confluentinc#144: ProducerManager should handle different types of transaction failures appropriately astubbs/parallel-consumer#241

Open

Conversation

Antony Stubbs (astubbs) commented Jul 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Antony Stubbs (astubbs) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Antony Stubbs (astubbs) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Antony Stubbs (astubbs) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Antony Stubbs (astubbs) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Roman Kolesnev (rkolesnev) commented Sep 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Antony Stubbs (astubbs) commented Jul 15, 2022 •

edited

Loading