DanielLeens commented on PR #10874: URL: https://github.com/apache/seatunnel/pull/10874#issuecomment-5611964786
Thanks @SEZ9. Confirmed again via the raw API body: comment 5601185764 is not cut off after "queue.poll() ... is outside" — that's the same rendering/collapse pattern we've hit a couple of times now. Reposting the full F3 paragraph and what follows it: > **F3 (queue-poll vs. `closeTable()` writer-removal race) — Not fixed, explained, deferred.** I traced the synchronization myself: `queue.poll()` (`MultiTableWriterRunnable.java:117`) is outside any lock, but `queueElement.process(this)`/`writeRow` is inside `synchronized (this)`, and `closeTable()`'s writer removal is inside `synchronized (runnable.get(i))` — the same monitor. A row dequeued just before `closeTable` removes its writer can still lose that race and hit a null `tableIdWriterMap.get(...)`, throwing under fail-fast policy. It's real but narrow, and it surfaces as a checkpoint-recoverable task failure, not silent data loss. Agreed a per-table in-flight counter spanning both classes is its own follow-up, not a one-line fix — but per my review, please file it as a tracked issue linked from this PR before merge, not left only in review history. > > Separately, while re-checking the current diff to answer this: `expectedCloseTableEventCounts.merge(event.tableId(), expectedSourceEventCount, Math::max)` in `handleCloseTableEvent` is still present at this head unchanged. That's the same merge strategy behind the stale-over-count-pin concern raised in earlier rounds (a stale, larger `expectedSourceEventCount` from an older reader generation can pin `requiredCount` above what will ever arrive, so the table never closes and just silently falls back to close-at-task-end with no warning log). F4/F6 above fixed the straggler-leak half of that area but is a distinct code path from the `Math::max` pin itself, and I don't see it called out in my own Issue Summary in the last full review — flagging so it isn't accidentally dropped from tracking. I don't have a fresh repro confirming it's still live given how much else changed around it, so I'm not reopening it as a numbered issue this round without doing that work properly — I'll gi ve it a full re-trace in the next full review pass rather than assert its status here on a reply-only round. > > Bar for merge from my side stays: F2 written into the PR description (blocker), F3 filed as a tracked follow-up issue (recommended, non-blocking), and the CI picture cleaned up (sync + rerun, since the current failures are all in modules this PR doesn't touch). Once those land I don't expect further structural concerns from me. So to be explicit about the deferral rationale you asked for: F3 stays deferred (not fixed in this PR) because the fix is a per-table in-flight counter spanning both `MultiTableSinkWriter` and `MultiTableWriterRunnable`, which is new shared state and a new decrement call, not a one-line change — I'd rather it get its own PR and its own review than get folded into this one's diff. It's acceptable to defer specifically because the failure mode is a checkpoint-recoverable task failure (fail-fast throw, job restarts and retries) rather than silent data loss, so it doesn't block merge as long as it's filed as a tracked follow-up issue linked from this PR, which is still outstanding. Still waiting on the F2 PR-description note and the F3 follow-up issue link before I'd consider the merge bar clear from my side. -- 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]
