SEZ9 commented on PR #10874: URL: https://github.com/apache/seatunnel/pull/10874#issuecomment-5578123756
Thanks @DanielLeens for going through these point by point — that lines up with my read, so let me confirm where we stand and what I'd like to see next. **F5 (ordering gap)** — Agreed it's inherent to event ordering and a much narrower window than the original bug. I'm fine keeping it non-blocking, but since the `requiredCountOnFile` null/`<=1` fall-through at `MultiTableSinkWriter.java:583` → `markTablePendingClose` at line 597 is still open on this head, please at least add a comment there documenting the known gap, or a short note in the PR description, so it's not rediscovered later. **F5/F7 (late row bypasses `MultiTableFailurePolicy`)** — Glad we agree it's an inconsistency rather than an intentional contract. Concrete ask: move the already-closed-table check at `MultiTableSinkWriter.java:658-663` behind the same `failurePolicy.continueOtherTables()` gate at line 668 that the missing-primary-key path at 677-686 uses, so a late row for a closed table is dropped/logged under the continue policy instead of hard-failing the task. A unit test covering the continue-policy case would be great. **F1/F8 (unbounded wait in `snapshotState`)** — Thanks for raising the severity. Ask: put a bound on `waitUntilTableQueueDrained` (`MultiTableSinkWriter.java:812-822`) — either a deadline with a clear failure/log after it expires, or fall back to not closing the table in that snapshot and retrying next time — and while there, propagate the interrupt properly instead of the `RuntimeException` wrap at 818-821 given the method declares `IOException`. **F2 (no final `prepareCommit` before closing sub-writers)** — OK to carry as a non-blocking follow-up for `closeTable()` at `MultiTableSinkWriter.java:840-881`, as long as it's tracked somewhere visible. Could you note it in the PR description so it isn't lost? **F4/F6** — Agreed the duplicate/late-event map leak half is still open; the cleanup for straggler events after a table is closed should land in this PR. On the merge/get atomicity half, your comment appears to have been cut off mid-sentence after the `handleCloseTableEvent` (`MultiTableSinkWriter.java:604-606`) `ConcurrentHashMap#merge` point — could you post the rest of that reasoning? I don't want to assume the conclusion either way. **F3 (in-flight row between queue poll and write)** — I didn't see this one addressed in your reply; is it still open on this head, or did I miss something? Once F5/F7, F1/F8 and the F4/F6 leak cleanup are in, I'm happy to do another pass. <!-- streview-comment:885 --> -- 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]
