DanielLeens commented on PR #10874: URL: https://github.com/apache/seatunnel/pull/10874#issuecomment-5578488329
Thanks @SEZ9, going through these. **On the "cut off mid-sentence" concern** — I just re-pulled the raw body of that comment via the API (comment id `5569403658`) and it is not truncated: it runs the full 3744 characters through the F4 paragraph, the F3 paragraph, and the closing "Net:" summary. So to remove any ambiguity, here is the F4 reasoning again in full: > On the non-atomic merge/get: I looked at `handleCloseTableEvent` (`MultiTableSinkWriter.java:604-606`) — `ConcurrentHashMap#merge` guarantees the key exists the instant it returns, so on a single calling thread the immediately-following `get()` can't NPE. The only way it could is if another thread removes that same key (via `markTablePendingClose`/`closeTable` cleanup) between this thread's `merge()` and `get()` — which would require `handleCloseTableEvent` itself to be invoked concurrently for the same table, and unlike the writer-mutation paths in this class it isn't `synchronized`. I haven't confirmed whether the event-dispatch caller can actually deliver concurrently for one table, so I can't call this a live bug yet — worth the author confirming that threading contract. That's the complete thought — nothing more was cut off after it. Looks like the same web-UI rendering/collapse artifact we ran into on #10551. **F3** — also already answered in that same comment, just easy to miss since it's the paragraph right before the summary: still open, not yet re-verified either way on my side. I said I'd check `MultiTableWriterRunnable`'s own processing loop before the next round rather than guess, and that's still true today — I haven't done that check yet, so please keep it open until I report back with an actual answer. On the concrete asks (F5 comment/PR-description note, F5/F7 moving the closed-table check behind the `continueOtherTables()` gate plus a continue-policy test, F1/F8 bounding `waitUntilTableQueueDrained` and fixing the interrupt handling, F2 PR-description note) — all four line up with what I already have tracked, and the F5/F7 and F1/F8 asks in particular match the two items we'd both flagged as correctness-affecting rather than pure hardening. Those are implementation asks for @davidzollo though, not review disagreements, so I'll leave the "how" to the author. Once a commit lands covering F5/F7, F1/F8, and the F4/F6 leak cleanup, I'll do a fresh full pass — including finally closing out F3 with an actual answer instead of a placeholder. -- 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]
