Repository navigation
feat(core): a commit-failure seam, so the application decides instead of PC always terminating #317
Description
The gap
When a commit ultimately fails, PC's only response is to terminate: the exception escapes controlLoop, supervisorLoop records it as the failure cause, and the whole instance closes. The application gets no say, and finds out afterwards through getFailureCause().
Users have already worked around this in the field. On confluentinc/parallel-consumer#833, ndqvinh2109 reported:
we got the same issue and PC has been closed unintentionally. We have applied a work-around to try catch the
controlLoopfunction so that the exception won't propagate tosupervisorLoopand close the PC entirely.
That is a user patching library internals to get a decision PC does not offer.
What Kafka does instead
Kafka's client throws TimeoutException - retriable in its taxonomy - and hands the decision to the caller. It bounds the operation (default.api.timeout.ms over request.timeout.ms), reports honestly, and lets the application choose: retry, back off, alert, or stop. PC borrows the two-level timeout model but not the seam.
The counter-argument is on record too, from rkolesnev on the same issue:
if it cannot commit data - then polling will fail as well and processing anything that was previously polled is just going to cause more side-effects / duplicates... I don't think that indefinite retry is an answer there either.
That is a fair case for defaulting to shutdown. It is not a case for making it the only option - which is what a seam changes.
Proposal
A user-supplied hook invoked when a commit has exhausted its budget, before PC decides to close, receiving the failure and enough context to act (offsets in play, attempts made, elapsed). It returns a decision - at minimum SHUT_DOWN (today's behaviour, and the default) versus CONTINUE.
Design questions this needs answered, not assumed:
- What does
CONTINUEmean for correctness? Uncommitted offsets stay dirty and are re-committed next cycle, so the risk is unbounded reprocessing on restart rather than loss - but that needs stating, not implying. - Which failures reach the hook? A commit timeout is one thing; an authorization failure is not obviously retriable by continuing.
- Where does it run? The broker-poll thread is the only producer of commit responses, so a slow hook there stalls exactly the thread whose health is in question.
- Does it subsume
PCRetriableException's shape, or is it a separate concern?
Relationship to #177 / confluentinc#833
Deliberately not a duplicate. #177 is the reported defect - a commit timeout reporting the wrong cause, plus the causes behind it - and is being addressed by #204 (reporting, and the retry budget) and #29 (the AB-BA deadlock). This issue is the feature embedded in that thread by a second reporter: the choice PC never offered, which is why they patched controlLoop themselves.
Fixing the defect does not remove the request. Even with a correct, honestly-reported commit failure, PC still terminates and the application still has no say.
Activity
feat(core) #177: name the budget that ran out, and stop charging SASL…
docs(inflight) #317: rank the commit-failure seam top, and record the…
fix(core) #177: make commitSync tell the truth about its budgets
docs(inflight): the follow-ups this PR found and did not fix
fix(core) #177: make commitSync tell the truth about its budgets
docs(inflight): the follow-ups this PR found and did not fix
fix(core) #177: make commitSync tell the truth about its budgets
docs #317: apply the round-2 review's five remaining findings, walked…
feat(core) #317: the commit-failure seam's public surface - handler, …
docs(inflight) #317: record the draft PR and the branch rename in the…
feat(core) #317: route sync-path budget exhaustion to the handler ins…
feat(core) #317: pause-intake mode, composed with the user's own pause
fix(core) #317: a CONTINUE decision consumes the pending commit command
feat(core) #317: the transactional commit path gains the budget, and …
1 remaining item
feat(core) #317: make a continuing-but-failing instance impossible to…
refactor(core) #317: simplification pass over the seam - reuse, and c…
docs(core) #317: the seam's user documentation, and the async gap tra…
fix(core) #317: apply the code review's mechanical findings - recover…
test(core) #317: split the 1456-line seam test into five classes on i…
fix(core) #317: a revocation callback declines the commit lock, and a…
docs(core) #317: disclose the abort lane's output loss, and square th…
docs #317-adjacent: record the inherited phantom-commit-success defec…
fix(core): the revoke sweep continues past a stateless partition, and…
fix(core) #248: a permanent async commit failure is an ERROR, because…
fix(core) #248: an async commit counts as committed when the broker a…
refactor(fluent) #504: the commit-failure policy leaves Milestone A -…