SEZ9 commented on PR #10874: URL: https://github.com/apache/seatunnel/pull/10874#issuecomment-6008621939
Following up on the consolidated state of this review — the remaining items are all on the author's side, and none of them are open disagreements between reviewers: - **F2 (blocker)** — `closeTable()` (`MultiTableSinkWriter.java:769-838`) still closes sub-writers without an interposed `prepareCommit`, so uncommitted 2PC data for a closed table is dropped. Please add a short paragraph to the PR description that states this behavior explicitly and explains either why it is acceptable or what mitigates it. This is the one item I consider a hard merge bar. - **F3 (deferral accepted)** — Deferring the queue-poll / `closeTable()` writer-removal race is fine since it surfaces as a checkpoint-recoverable task failure rather than silent data loss. The follow-up issue still needs to be filed and linked from the PR description, though; I don't see that yet. - **F4/F6 `Math::max` question** — Still open: can a stale, larger `expectedSourceEventCount` from an older reader generation pin `requiredCount` in `handleCloseTableEvent` above what will ever arrive, silently falling back to close-at-task-end? Either a warn log on that path or a sentence in the PR description explaining why it can't happen resolves this for me. On the branch itself: the current head `ce9c17406b7` is `diverged` from `dev` (`ahead_by=21`, `behind_by=122`) and GitHub now reports `mergeable_state` as `dirty`, i.e. real merge conflicts rather than just a stale signal. Please sync with the latest `dev`, resolve the conflicts, and rerun CI — until that's done I can't meaningfully evaluate the CI result. Once F2 is documented, the F3 issue is linked, the `Math::max` question is answered, and the branch is synced with a clean run on top, I'll do another full pass. <!-- streview-comment:1548 --> -- 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]
