DanielLeens commented on PR #10874: URL: https://github.com/apache/seatunnel/pull/10874#issuecomment-5569403658
Thanks for the careful follow-up, @SEZ9 — going through these in order. **F5 (ordering gap) — you're right, and it's not overlooked.** This is exactly the residual gap I called out as Issue 1b in my last review: when an un-attributed event is the *first* event seen for a table (`requiredCountOnFile` is `null` or `<=1` at `MultiTableSinkWriter.java:583`), it falls straight through to `markTablePendingClose` at line 597 with nothing on file yet to compare against. I rated it Low/non-blocking because it's inherent to event-ordering ambiguity (you can't know a table is multi-reader until at least one reader says so) and is a much narrower window than the original bug, but it's genuinely still open — I haven't seen a fix for it on this head. **F5/F7 (late row bypasses `MultiTableFailurePolicy`) — you've found a real, previously-untracked gap.** I checked `write()` directly: the already-closed-table check at `MultiTableSinkWriter.java:658-663` throws `IOException` unconditionally, *before* the `failurePolicy.continueOtherTables()` check at line 668 that every other write-time failure path in this method (e.g. the missing-primary-key case at 677-686) is routed through. That's a real inconsistency, not an intentional contract — I don't see anywhere this was discussed before. I'm adding it to my tracked list for the next round; thank you for catching it. **F1/F8 — also a real refinement, and worse than I rated it.** `waitUntilTableQueueDrained` (`MultiTableSinkWriter.java:812-822`) is indeed an unbounded `while (hasQueuedRows) Thread.sleep(100)` loop with no deadline, and it's invoked from `closeTable()` inside the `snapshotState()` path, so a queue that never drains would stall checkpointing indefinitely, not just leak an exception type. I'd previously only flagged the `RuntimeException`-vs-declared-`IOException` wrapping (line 818-821) as Issue 12/Low; I'm raising that to Medium to reflect the checkpoint-blocking angle you raised. **F2 — tracked, still open.** Matches Issue 7 in my last review (`closeTable()` closes sub-writers without a final `prepareCommit`, `MultiTableSinkWriter.java:840-881`); carried forward as a non-blocking follow-up. **F4/F6 — half confirmed, half needs more digging.** The tracking-map-leak-on-duplicate-event half matches Issue 4, still open. 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. **F3 — I haven't re-verified this one yet** (it needs a look at `MultiTableWriterRunnable`'s own processing loop, not just `MultiTableSinkWriter`), so I don't want to guess either way here; I'll check it before the next round rather than give you an unverified answer. Net: F1(1b) confirmed-still-open-by-design, F5/F7 and F1/F8's checkpoint-blocking angle are new tracked issues, F2/F4(leak half) are existing tracked issues, F4(NPE half) is an open question pending the threading contract, F3 is pending my own re-check. Agreed F5/F7 and F1's boundedness should be resolved before merge given they're both correctness-affecting, not just hardening. -- 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]
