SEZ9 commented on PR #10874: URL: https://github.com/apache/seatunnel/pull/10874#issuecomment-5595384765
Thanks @DanielLeens for the detailed re-trace on `ce9c17406b7` (on top of `9c1f7f587f67`). Agree with your reading of the lifecycle: closing in `closePendingTables()`/`closeTable()` from `snapshotState()` after the barrier is the right place. On **F5/F7**, the `failurePolicy.continueOtherTables()` check at `MultiTableSinkWriter.java:677-691`, mirroring the branches at `:696-699` and `:706-714`, is what was asked for. I'll treat F5/F7 as resolved once a test covers the late-row-under-continue-policy path. Your review appears to be cut off after "Under continue-policy" — you mention three fixes but only the F5/F7 one is visible. Could you re-post the remainder so we know which of the other findings (F1/F8, F2, F3, F4/F6) you verified as fixed on this head? For whichever of those remain unaddressed, the asks from the previous round still stand: - **F1 / F8** – bound the wait in `waitUntilTableQueueDrained()` (timeout + backoff, or a latch signalled by the drain loop) so a stuck sub-writer can't hold the checkpoint barrier forever, and surface `InterruptedException` as an `IOException` / restore the interrupt flag instead of wrapping it in `RuntimeException`. - **F2** – `closeTable()` should run a final `prepareCommit` and hand the result to the normal commit path before closing sub-writers, so 2PC sinks don't abandon the closed table's uncommitted transaction. - **F3** – track in-flight rows (e.g. a per-table in-flight counter, or a drained flag set by the consumer after the write completes) so `closeTable()` can't race a row that was polled but not yet written. - **F4 / F6** – make the close-vote accounting in `handleCloseTableEvent` (`MultiTableSinkWriter.java:573`) atomic rather than `merge` + `get`, and drop/log events for an already-closed table instead of repopulating the tracking maps. Summary: (1) the missing part of your review, (2) fixes or explicit pushback on any of F1/F8, F2, F3, F4/F6 not already covered, and (3) unit coverage for the closed-table paths, including the F5/F7 continue-policy branch. Happy to take another pass as soon as those land. <!-- streview-comment:921 --> -- 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]
