DanielLeens commented on PR #11903:
URL: https://github.com/apache/seatunnel/pull/11903#issuecomment-5370796591
Thanks for the close read, @SEZ9. I rechecked issues 1 and 5 against the
current head (`2b7233755ebc`) and neither holds up against the code as written
— my approval stands, but let me walk through why:
**Issue 5 (Signal not covered by the prepareClose short-circuit) — not
accurate.** All three non-barrier branches in `received()`
(`SinkFlowLifeCycle.java:300-329`) gate on `prepareClose` identically:
`SinkFlowLifeCycle.java:307` (SchemaChangeEvent), `:313` (Signal), `:319` (data
record) each do `if (prepareClose) { return; }` before dispatching. Signal is
short-circuited exactly the same way as the other two — nothing in the current
code lets signals through after a close barrier.
**Issue 1 (abortPrepare only on prepareCommit failure) — not accurate.** The
`catch (Exception e)` at `SinkFlowLifeCycle.java:847` wraps the entire
snapshot-processing `try` block (`:807-846`), not just the `prepareCommit` call
at `:809`. That block also covers `drainCollectedTerminalWriteOutcomes()`,
`flushDeferredTerminalWriteOutcomes()`, `sealCheckpointMetrics()`,
`writer.snapshotState()`, `runningTask.addState(...)` (state serialization),
and the `sendToMember(...).join()` call to the aggregate committer. Any
exception from any of those steps reaches `abortPreparedWriter`, so the
existing Javadoc wording ("A failure invokes abortPrepare() before it is
propagated") is actually the more accurate description — narrowing it to "only
on prepareCommit failure" would make the doc less correct than it already is.
Issues 2, 4, 6, and 7 are reasonable, non-blocking precision suggestions I'd
be glad to see picked up in a follow-up doc pass — the `@throws
RuntimeException` wrapping note (Issue 2) and the
caller-enforced-not-self-enforced ordering point (Issue 4) are both fair. On
Issue 3: the current phrasing ("Serialized states are deserialized... An empty
state list creates a new writer") already scopes "state list" to the
deserialized list from the preceding sentence rather than the raw
`actionStateList` — I agree it reads a little tight and could be split into two
sentences, but it isn't factually wrong given `restoreState`'s actual branch
(`SinkFlowLifeCycle.java:399`) keys off the deserialized `states`, not the
parameter.
Since this is a documentation-only PR and its core behavioral claims check
out against the code, I don't think 1/5 should block, and 2/3/4/6/7 read as
polish rather than correctness gaps. @goutamadwant no action needed on your end
unless you want to take a pass at the wording tightening.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
