SEZ9 commented on PR #10874: URL: https://github.com/apache/seatunnel/pull/10874#issuecomment-5629370063
Following up on the reposted text of comment 5601185764 so the remaining items are in one place. **F3 (queue-poll vs. `closeTable()` writer-removal race)** — I agree with the analysis: `queue.poll()` in `MultiTableWriterRunnable` sits outside the monitor while both `writeRow` and the writer removal in `closeTable()` are inside `synchronized (runnable.get(i))`, so a row dequeued just before removal can still hit a null `tableIdWriterMap.get(...)`. Since it surfaces as a recoverable task failure rather than silent loss, I'm fine deferring the per-table in-flight counter that would span both classes. Ask: please open a tracked follow-up issue describing the race and link it from the PR description before merge, so it doesn't live only in review history. **F2 (no final `prepareCommit` before sub-writer close in `closeTable()`)** — this remains the blocker. Please add a short paragraph to the PR description stating that `closeTable()` closes sub-writers without a final `prepareCommit`, what that means for 2PC sinks, and why the current behavior is acceptable (or what the mitigation is). **F4/F6 (close-table event accounting)** — the straggler-leak cleanup looks addressed, but `expectedCloseTableEventCounts.merge(event.tableId(), expectedSourceEventCount, Math::max)` in `handleCloseTableEvent` is still the same strategy. Could you confirm whether a stale, larger `expectedSourceEventCount` from an older reader generation can pin `requiredCount` above what will ever arrive? If it can, the table would silently fall back to close-at-task-end. At minimum a warn log on that path would help; if you believe it can't happen, a sentence explaining why in the PR description is enough for me. Finally, please sync with the base branch and rerun CI so we have a fresh picture on the current head. Once F2 is documented, the F3 follow-up issue is linked, and the `Math::max` question is answered, I'm good to move forward. <!-- streview-comment:970 --> -- 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]
