docs #171: document that a bare close() does not drain - #213
Conversation
A bare close() is closeDontDrainFirst() - the non-draining mode - and that is also what try-with-resources calls. Nothing in the README or the Javadoc said so, and the assumption most people bring from Closeable is the opposite, so queued-but-unstarted records get silently dropped and redelivered. Adds a "Shutdown and Close Modes" README section covering DRAIN vs DONT_DRAIN, which one bare close() picks, and how the two timeouts apply, plus a tagged CoreApp example. Also fixes two things the Javadoc got wrong: close() now states the non-draining default up front, and the Duration overload documents that it overrides shutdownTimeout only - drainTimeout always comes from the options. Note the defaults differ: shutdownTimeout is 10s, drainTimeout is 30s. 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 |
|
Claude finished @astubbs's task in 3m 22s —— View job Claude finished reviewing this PRTodo list
SummaryThis is a docs/Javadoc-only PR closing #171, and it's well done. I verified every factual claim in the PR body against the actual code in
I also checked build mechanics:
Minor nit (non-blocking)
No other issues found. Scope is clean (docs/Javadoc + one example method, no behavior changes), and the PR description's corrections against the original issue are all accurate. |
✅ 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). |
|
Claude finished @astubbs's task in 1m 34s —— View job Claude finished reviewing this PRTodo list
SummaryThis is a docs/Javadoc-only PR closing #171, and it's well done. I verified every factual claim in the PR body directly against the code:
Build/mechanics checks:
Minor nit (non-blocking)
No other issues found. Scope is clean (docs/Javadoc + one example method, no behavior changes), and all of the PR description's corrections against the original issue check out against the code. |
…wording Review asked why CoreApp.closeModes() is there, since nothing calls it. Adds Javadoc naming the mechanism: the tag is pulled into the README by the asciidoc template plugin, so compiling this file is what stops the published snippet going stale. Also takes the review bot's point on close(Duration, DrainingMode): saying drainTimeout "bounds the draining phase" invites exactly the misreading this PR set out to fix, since the drain loop has no internal deadline. Now says it is a term in the overall budget, matching the README. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @astubbs's task in 2m 10s —— View job Claude finished reviewing this PRTodo list
SummaryI re-reviewed this PR against the current HEAD (
I re-verified the rest of the PR's factual claims still hold at this HEAD:
No new issues found. This is a clean, well-scoped docs/Javadoc-only PR — both prior review comments have been addressed with precise, accurate wording rather than surface-level fixes. |
|
Taking the non-blocking nit - it was a fair catch, and precision was the whole point of the PR.
Reworded in 3c60bd3 to match the README rather than contradict it:
Also addressed the review comment on |
#213 documented on DrainingCloseable#close() that a bare close() is the non-draining one. This comment was written before that existed and explained the delegation itself, which is now a second copy of a fact with a proper home. Keep only what the javadoc cannot carry: why teardown wants the non-draining close specifically - it runs on the failure path, so it must not be able to hang on in-flight work against a consumer still being made to misbehave. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
1 participant
Closes #171 (#171, mirror of confluentinc#642).
The trap
A bare
close()iscloseDontDrainFirst()- the non-draining mode. That is also what try-with-resources calls, since the processors areCloseable. The assumption most people bring fromCloseableis the opposite, so queued-but-unstarted records get dropped, never committed, and redelivered to whoever picks up the partition next.Nothing in the README said anything about shutdown modes at all - the only mention was the feature-list bullet "Clean draining shutdown cycle". The Javadoc on
close()said "Close the consumer, without draining", which is technically true and easy to read straight past.What changed
Shutdown and Close ModesREADME section (insrc/docs/README_TEMPLATE.adoc;README.adocregenerated, not hand-edited). Covers DRAIN vs DONT_DRAIN, which one bareclose()picks, and how the two timeouts apply, with a taggedCoreAppexample.DrainingCloseable.close()Javadoc now leads with "WITHOUT draining" and names try-with-resources explicitly. That is where most people will actually hit this, reading in an IDE.close(Duration, DrainingMode)@param timeoutnow says it overridesshutdownTimeoutonly -drainTimeoutalways comes from the options. The old wording ("override timeout set in ParallelConsumerOptions") implied it set both.Corrections found while verifying against the code
The issue's summary was wrong or incomplete on several points, and the doc reflects the code rather than the issue:
shutdownTimeoutis 10s,drainTimeoutis 30s (ParallelConsumerOptions.java:524,531). The issue implied 30s applied to both.drainTimeoutis not a deadline on the drain phase. It is a term in the overallwaitForClosebudget; the draining loop itself has no internal deadline and runs until the backlog is empty. Overrunning the total budget throws aTimeoutExceptionout ofclose()rather than returning quietly.innerDoClose()callscommitOffsetsThatAreReady()on both paths, so the distinction is not "commits vs doesn't". What DONT_DRAIN loses is the queued backlog, which is cleared from the pool queue and therefore stays incomplete in the offset encoding.Scope
Docs and Javadoc only - no behaviour change. No CHANGELOG entry: per
AGENTS.md, changelog sections are generated from the commit log at release time, and this is not user-observable behaviour.🤖 Generated with Claude Code
Checklist
N/A- documentation and Javadoc only, no behaviour change. The newCoreApp.closeModes()example is compile-checked by the build, and the README include tag would break generation if it drifted.N/A- no CI, workflow or runner changes.