SEZ9 commented on PR #10874: URL: https://github.com/apache/seatunnel/pull/10874#issuecomment-5564610420
Thanks @DanielLeens for re-reviewing the full head at `9c1f7f587f67` rather than just the delta from `b95f79ae61b6`, and for confirming that the fast-path close bypass and the premature marking-closed/eviction issue are now covered by regression tests. The `requiredCountOnFile` guard matches my reading of the intent: it stops an un-attributed event from short-circuiting an active aggregation. As I read the quoted snippet, though, an un-attributed event still falls through to `markTablePendingClose` when no attributed count > 1 is on file, so I don't think F5 is resolved yet; happy to be corrected if I'm missing a hunk. On F5/F7: a late row for an already-closed table hard-fails with `IOException` instead of going through `MultiTableFailurePolicy`. Please either route it through the policy or explain why a hard failure is the intended contract. The other points from my previous pass in `MultiTableSinkWriter.java` don't appear to be addressed in this thread, so a short status on each would help: - F1/F8: `waitUntilTableQueueDrained()` is an unbounded busy-wait invoked from `snapshotState()`, which can hold the checkpoint barrier indefinitely; it also wraps `InterruptedException` in `RuntimeException` while the method declares `IOException`. A bounded wait (or a timeout that fails the checkpoint cleanly) plus restoring the interrupt flag would resolve both. - F2: `closeTable()` closes sub-writers without a final `prepareCommit`, so for 2PC sinks uncommitted transactional data of that table is dropped. Please confirm whether a commit step is issued before the writer is closed, or add one. - F3: `hasQueuedRows()` only looks at queue contents, so a row polled but not yet written can still be in flight when the sub-writer is closed. This needs an in-flight marker or a drain handshake with the worker thread. - F4/F6: the close-event accounting uses a non-atomic merge/get pair (possible NPE), and straggler or duplicate close events for an already-closed table repopulate the tracking maps without cleanup. If any of these were addressed in `9c1f7f587f67` and I've missed it, point me to the relevant hunk and I'll re-check. Otherwise, F1/F2/F3 are the ones I'd like resolved before merge; F4–F8 I'm fine tracking as follow-ups if you prefer, as long as that's noted in the PR description. <!-- streview-comment:864 --> -- 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]
