Repository navigation
docs #300: record the misnamed produce-send failure, and preserve the swept PR heads only upstream had - #301
Conversation
…duce-send failure Raised while reading back the #261 fix. The surviving throw in ProducerManager's send callback is reachable in ordinary operation - client-side rejection of a result record the user's function returned, RecordTooLargeException being the case the new IT exercises - so it is an expected state, not an internal fault. Calling it InternalRuntimeException makes every reader re-derive that whole chain (which path is reachable, why the async one is not, what actually throws) before concluding it is ordinary failure handling. That derivation was done from source and kafka-clients bytecode during review and was recorded nowhere. Preferred fix is non-breaking: throw a specific subclass so existing catch (InternalRuntimeException) keeps working while the type states the situation. Renaming the base type would be user-visible and is called out as belonging in the breaking-changes section instead. Also corrects this note's own account of confluentinc#291. It is a PR, not an issue, it implements confluentinc#242, and both concern exceptions the USER FUNCTION throws - not send-failure classification. Reading them as cover for send retries was wrong; that work sits under #141 / #149, epic #239. Records that both are already accounted for, so neither gets re-mirrored: confluentinc#291 was closed unmerged on 2023-06-15, in the PR half of the administrative sweep, and is carried in upstream-map.yaml under sweep-2023-admin-closure - it has no dedicated fork issue only because that cohort mirrored the 28 swept issues, with the 35 swept PRs held in the map. confluentinc#242 is not sweep-affected at all, closed as completed by astubbs in 2022. Notes the one real gap: #239's "Prior work" cites confluentinc#366 from that same cohort but not confluentinc#291, which is equally prior art for the retry half of that epic. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
No new clones introduced by this PR. Powered by astubbs/duplicate-code-cross-check |
✅ SpotBugs ReportNo bugs found (new bugs only — baseline from base branch excluded). |
🧪🔒 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5416f1e99f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…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>
…en the failure set Six of ten Codex findings were valid; three were already fixed by the previous commit and one is rebutted on the repo's own rule. - The retryable half WAS built. PCRetriableException is public and AbstractParallelEoSStreamProcessor honours it on the user-function failure path, so "terminal-vs-retryable exceptions were never built" was wrong. What is missing is only the terminal half: nothing can say "stop, this will never succeed". - The synchronous callback path is not limited to bad result records. KafkaProducer#doSend raises everything before the accumulator there, so metadata and authorization failures (TimeoutException, TopicAuthorizationException - both ApiException) reach it as well. The naming has to cover environment failures, not just oversized records. - Adds the // TODO(refactor) marker at the sendCallback site. docs/refactoring.md requires capture at the point of noticing; a backlog entry with no code-local marker is invisible to grep. docs/todo-index.md regenerated (85 markers). - README_TEMPLATE.adoc is at src/docs/, not the root - the citation pointed at a path that does not exist. - Orphan tags are not reachable from any branch, so a plain git fetch in an older clone does not bring them down; names git fetch --tags, and notes that verifying via ls-remote needs no fetch at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
changed the title
|
@claude review this Focus on factual accuracy over style: this PR is documentation whose entire value is that its claims are true. Worth checking hardest:
A Codex review already ran on an earlier head; all ten threads are resolved, six by fixes in |
|
Claude finished @astubbs's task in 5m 55s —— View job Review of PR #301
Focused on factual accuracy per the request. Findings below, ordered by the three areas asked about. 1. Six swept PR heads (
|
| PR | Table SHA prefix | Actual headRefOid |
Actual author | Actual head owner |
|---|---|---|---|---|
| #22 | ba6b71f1 |
ba6b71f10359c2f75aacedf7d15e90d2cf7c9cc5 |
astubbs ✓ | confluentinc ✓ |
| #204 | 02ab3289 |
02ab3289463648ee3c869014cfcc2f8e5ca04ba9 |
astubbs ✓ | confluentinc ✓ |
| #270 | 007ae090 |
007ae09068468b526debc3c9972c44ad6c769a66 |
astubbs ✓ | confluentinc ✓ |
| #405 | 77a021ee |
77a021ee10916b04013f158a49e7ceb463d3d8e2 |
astubbs ✓ | confluentinc ✓ |
| #443 | 4533f6d8 |
4533f6d8d3fd590d273243d1e725df70d89062f0 |
Robbie-Palmer ✓ | confluentinc ✓ |
| #506 | 33d93af0 |
33d93af0919616875a5c443a616aade8a2562f1b |
astubbs ✓ | confluentinc ✓ |
All six SHAs, authors, and the (corrected, post-Codex) "raised from confluentinc:<branch>, not a contributor fork" claim check out exactly — right down to #443's head branch literally being pyallel-consumer (the typo the doc calls out) and living on confluentinc, not Robbie-Palmer's own fork. upstream-map.yaml's longer SHA prefixes (ba6b71f10, 02ab32894, etc.) are consistent extensions of the same commits. The 35-PR list in upstream-map.yaml:366 also counts to exactly 35, and the six preserved PRs are a subset of it.
Also spot-checked the reclassification of confluentinc#242/#291: #242 is closed COMPLETED 2022-10-21 and its body is explicitly about a user function rethrowing a retriable exception ("User should be able to catch exception, and rethrow a retriable exception"); #291 is closed unmerged 2023-06-15 (matches the sweep date) and references #242. Both support the corrected claim that these are about user-function exceptions, not send-failure classification. Also confirmed astubbs#239's "Prior work" section cites only confluentinc#366, not #291 — the gap the PR body flags for later is real.
One thing I could not verify: whether the archive/upstream-pr-* tags actually exist on this fork and peel to those SHAs, or whether the other 29 heads are really reachable from a fork branch. My tool grant on this route is git rev-parse only (no git fetch/git ls-remote, and gh api against astubbs/parallel-consumer isn't in the allowlist either) — see REVIEW_TOOL_ALLOWLIST in .github/workflows/claude.yml. Given how precisely the upstream-side facts check out, I have no reason to doubt the fork-side ones, but I couldn't independently confirm them this pass.
2. ProducerManager.sendCallback observability claim
Read ProducerManager.java:73-99 and cross-checked against KafkaProducer#doSend / ProducerBatch#completeFutureAndFireCallbacks. Both halves of the claim hold:
doSend'scatch (ApiException e)block invokes the callback synchronously, beforetransactionManager.maybeTransitionToErrorState(e)runs — so a throw there is observable and (pre-fix) skips that transition. This is the pre-accumulator path, and it's not limited to size rejections: anything raised in thatcatchblock (RecordTooLargeException, but alsoTimeoutException/TopicAuthorizationExceptionfrom metadata/authorization lookups, bothApiExceptions) reaches it. The widened wording indocs/refactoring.md:283-291(post-Codex-review fix ine404c99f) is accurate; the pre-fix "client-side rejections of the user's own result records" framing that Codex flagged would have been too narrow.- On the async, post-accumulator path,
ProducerBatch.completeFutureAndFireCallbackswraps each callback invocation intry { ... } catch (Exception e) { log.error(...) }— a callback throw is caught and logged there, never propagated. So "swallows callback throws on the async one" is correct.
Also verified the runUserFunction/runUserFunctionInternal correction: the catch (Exception e) block is in runUserFunction (AbstractParallelEoSStreamProcessor.java:1347-1367), wrapping the call to runUserFunctionInternal, which contains the usersFunction.apply call (line 1396) — matching the corrected text exactly, not the original (wrong) claim that Codex flagged.
3. Non-breaking subclass proposal
InternalRuntimeException (internal/InternalRuntimeException.java:25) is a plain, non-final extends RuntimeException. A new RecordPublishFailedException extends InternalRuntimeException is a standard-compatible addition: any existing catch (InternalRuntimeException e) still catches it, since Java catch matches on assignability, not exact type. I don't see a realistic case this breaks (an exact getClass() == InternalRuntimeException.class check would, but that's not an idiomatic pattern and nothing in the tree does it). The claim is accurate.
Other checks
./bin/todo-index.sh --checkpasses — theTODO(refactor)marker added atProducerManager.java:96-98is correctly indexed (85 markers, matching the author's stated count).PCRetriableExceptionis genuinely public, used, and honored on the user-function failure path (AbstractParallelEoSStreamProcessor.java:1357-1358), confirming the correction indocs/inflight/bug-poisoned-transaction-not-aborted-while-running.md.src/docs/README_TEMPLATE.adoc:347does carry theconfluentinc#291citation Codex said was missing at the root-level path — confirms that fix is real, not just claimed.- All ten prior Codex threads have visible, checkable resolutions in
e404c99f/27a97d4; I didn't find anything they raised that's still outstanding, and the author's declines (DEP-3 trailers, intermediate commit subjects) are backed by real policy text (docs/upstream.md:78: "Trailers are not enforced").
Bottom line
Everything I could check against ground truth (kafka-clients source behavior, upstream GitHub PR/issue metadata, in-repo cross-references) matches the PR's claims, including the corrections layered in during this review cycle. No factual inaccuracies found. The one gap is the fork-side tag/branch verification, which is outside what this route's tool grant permits me to check directly — worth a maintainer running git ls-remote --tags origin 'archive/upstream-pr-*' locally before merge if that assurance matters, but nothing here contradicts it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e404c99f8b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… finish the subclass proposal Both findings were valid and both were about a recorded procedure being wrong rather than prose being unclear. - The containment check must be restricted to origin/*. A full clone of this fork also carries an upstream remote (AGENTS.md, "gh defaults to the WRONG repo here"), and a bare `git branch -r --contains` searches every remote - so a head reachable only from upstream/* would read as preserved, which is precisely the case the archive exists for. Run as written by anyone with the standard two-remote checkout, the check would have found nothing to preserve. The recorded result is unaffected: this checkout has only origin, and the count was reconciled against a live ls-remote of origin's heads. - The subclass proposal was incomplete. A synchronous callback failure escapes produceMessages into processAndProduceResults, whose catch (Exception e) rethrows a fresh InternalRuntimeException, so the specific type would survive only as a nested cause and callers would still be unable to catch it. Says plainly that the subtype has to be preserved through that outer wrapper, or the refactor buys nothing but a better log line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9276f7ca7a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ranch deletion a loss event Round three of review. Two of the three findings are consequences of the previous round's fixes, which is worth recording. - Branch deletion is not a loss event. refs/pull/<n>/head is held by the base repository and keeps the commit after its branch is gone, so only loss of confluentinc/parallel-consumer itself puts these heads at risk. The previous commit replaced "a contributor's fork disappears" with "the branch behind a PR" - the same mistake in new clothing, since both assume the pull ref follows the branch. Says so explicitly now, so the next reader does not reintroduce it. - The six tag/PR/SHA tuples had two owners. Recording them in the manifest was the right fix for making the check redoable, but leaving the table here as well meant a corrected SHA could be updated in one copy while the other still read as authoritative - the exact drift this section exists to prevent. The tuples now live only in sweep-2023-admin-closure.preserved_heads; this section names the PRs and the tag-naming convention and points at the manifest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Restores the six-row table removed in the previous commit. The review finding was that the tag/PR/SHA tuples had two owners and a corrected SHA could drift - not that a table was the wrong shape. Deleting it was an over-correction; six rows are more scannable than a prose list. The table now carries only what does not change - upstream PR, author, and what the PR was. The tag name, target SHA and check date stay single-owner in sweep-2023-admin-closure.preserved_heads. The descriptions are the real upstream titles, fetched rather than recalled. The first draft of this table invented them and got three wrong, including pasting confluentinc#45's title onto confluentinc#22 - the same fabrication-from-memory this PR exists to correct. Embedded upstream refs are dropped from the titles so the issue-reference gate stays green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 306c5e6557
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Four of this round's five findings begin "fresh evidence after the earlier fix": each was caused by the previous round's patch. That is a method problem, not five unrelated defects. These edits touch prose whose claims interlock, and patching one sentence at a time leaves the neighbours contradicting it. This round rewrites the affected sections whole and re-reads them end to end. - The pre-accumulator claim was over-corrected. Widening "client-side rejections of result records" to "everything doSend raises" swapped one wrong exhaustive claim for its opposite: a serializer throwing SerializationException propagates without ever invoking the callback. The reachable set is exactly doSend's catch (ApiException) handler - narrower than any pre-accumulator failure, wider than oversized records. - git fetch --tags now names origin. On a branch tracking upstream the bare form fetches from there and leaves all six fork-only tags unavailable - the same two-remote trap already fixed for the containment query, reintroduced in the recovery command added to fix it. - The exposure statement is now one paragraph instead of three competing ones, and separates the two risks it kept conflating: upstream repository loss (the only way the pull refs go) versus deletion of the origin/* branch that contains one of the 29 - which is a fork-side risk and the one the recurring check must actually watch. - The manifest comment no longer contradicts itself. It said all 35 were reachable only through upstream pull refs and then that 29 were also on fork branches; it now states what each pass actually established. - The TODO(refactor) summary is back on one physical line. bin/todo-index.sh indexes only that line, so the wrapped version generated the truncated entry "expected state, not an" with the diagnosis and remedy dropped. Reading the section whole then caught a sixth instance the review had not flagged: a later paragraph still ended "the branch upstream is what matters", contradicting the corrected text above it. Fixed here rather than in round five. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24e36623ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…escape Round five, and the same conflation as round four in a new place. The entry claimed asynchronous failures "never reach" the callback. They do: ProducerBatch invokes the same onCompletion, so the log.error fires for effectively every failed send. What is unique to doSend's synchronous catch (ApiException) path is that a throw from the callback can escape to the caller; on the asynchronous path ProducerBatch catches and logs it. Only the second question governs the exception type, since only there is the thrown type observable to a caller - so the scope statement is unchanged in substance. But a refactor built on the wrong reachability model would drop logging for every asynchronous send failure, so the entry now states both questions separately and warns against that specific mistake. The javadoc #261 left in ProducerManager was already right about this - it says the throw was inert on the async path, not that the callback is not invoked. The error was introduced here while paraphrasing it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Description
Docs only - no code changes. Two records, both from reading back the #261 fix.
1.
InternalRuntimeExceptionmisnames the produce-send failureProducerManager's send callback throwsInternalRuntimeExceptionwhen a send fails in non-transactional mode. That is an expected operational state, not an internal fault: the callback runs on asynchronous failures too (ProducerBatchinvokes the sameonCompletion), but only ondoSend's synchronouscatch (ApiException)path can its throw escape to a caller - so that path, not callback reachability, governs the exception type.RecordTooLargeExceptionis the caseTransactionalPartialResultSetITexercises, but metadata and authorization failures (TimeoutException,TopicAuthorizationException) reach it too.The cost is rediscovery. A name saying "internal" sends every reader, human or agent, to re-derive that whole chain - which path is reachable, why the async one is not, what actually throws - before they can conclude it is ordinary failure handling. That derivation was done from source and kafka-clients bytecode during review and was recorded nowhere.
Recorded in
docs/refactoring.mdunder the existinginternal/ProducerManager.javasection, with:RecordPublishFailedException extends InternalRuntimeException) so existingcatch (InternalRuntimeException)keeps working while the type states the situation. The entry also records that the subclass alone is not enough -processAndProduceResultsre-wraps into a genericInternalRuntimeException, so the subtype must be preserved through that outer wrapper or callers still cannot catch it. Renaming the base type would be user-visible, so that route is pointed at the breaking-changes section instead - the subclass avoids needing it.exception. It is the send failure, not a user-code exception (those are caught byrunUserFunction, which wraps theusersFunction.applycall made inrunUserFunctionInternal), so it should besendFailure. The entry explicitly warns against naming it after user code, because that is the exact confusion it exists to stop.This also corrects the in-flight note added by #261, which mischaracterised confluentinc#291. It is a PR, not an issue; it implements confluentinc#242; and both concern exceptions the user function throws, not send-failure classification. Reading them as cover for send retries was wrong - that work sits under #141 and #149, epic #239.
Both are already accounted for and must not be re-mirrored: confluentinc#291 was closed unmerged on 2023-06-15 in the PR half of the administrative sweep and is carried in
upstream-map.yamlundersweep-2023-admin-closure; confluentinc#242 is not sweep-affected, closed as completed by astubbs in 2022.One small gap noted for later: #239's "Prior work" cites confluentinc#366 from that cohort but not confluentinc#291, which is equally prior art for the retry/terminal-exception half of that epic.
2. Six swept PR heads existed only upstream - now preserved
The tags in this section are already pushed; they are refs, so they do not appear in this diff. Only the
docs/upstream.mdwrite-up does.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>/headin the upstream repository - not a copy we control. If that repository goes, the commits go with it, and a mirror describing work whose diff no longer exists is close to useless.Checked each of the 35 against this fork's refs, with the containment query restricted to
origin/*- a full clone also carriesupstream, and a baregit branch -r --containswould report the upstream-only heads as preserved. 29 were already safe, raised from branches that still exist here. Six were reachable only from upstream - not on this fork, and not even present in a local clone:Each is tagged
archive/upstream-pr-<n>. The tag name, target SHA and check date live in one place -sweep-2023-admin-closure.preserved_headsinupstream-map.yaml- deliberately not duplicated into prose, so a corrected SHA cannot be updated in one copy while the other still reads as authoritative. The table carries only what does not change.confluentinc#443 was raised by an outside contributor, but its head lives on
confluentinc:pyallel-consumerlike the rest - that contributor's own fork is already gone and it made no difference. The exposure is loss ofconfluentinc/parallel-consumeritself; deleting the branch behind a PR is not a loss event, becauserefs/pull/<n>/headis held by the base repository.Annotated tags rather than branches - they are not swept by branch-cleanup tooling and read as archival rather than live work, matching the existing
refs/tags/archive/...convention. Each tag message carries the upstream title, author, head branch and closure date, so provenance survives without the upstream thread.Recorded as a recurring check, not a one-off - but explicitly not automated:
--auditcovers tracking and mirroring, not reachability, and would report clean with every archive tag deleted.Verification
git ls-remote, and each annotated tag confirmed to peel to the expected commit.bin/check-issue-refs.shpasses on the added lines.docs/,src/docs/and the 28 mirror bodies; it does not cover code comments, commit messages or branch names, and that caveat is stated where the finding is recorded.Checklist
docs/features/- N/A - no user-facing feature or option is added or changed; this records deferred work and preserves history