DanielLeens commented on PR #10306: URL: https://github.com/apache/seatunnel/pull/10306#issuecomment-5304917092
Update on the CI gate I flagged as purely procedural in my last review (Issue 18): that's changed, and it's not just "still queued" anymore. **What changed.** The fork's `Build` run (`31640721866`, https://github.com/zhangshenghang/seatunnel/actions/runs/31640721866) finally left the `queued` state after sitting there 45+ hours and completed with `conclusion=failure`, and the apache-side `Build` check now mirrors that (https://github.com/apache/seatunnel/runs/94262284587). This is a real completed run, not the stuck one I reported before, so it needs a real diagnosis rather than "wait for CI to start." **The actual failure.** Job `Run / unit-test (11, windows-latest)` (https://github.com/zhangshenghang/seatunnel/actions/runs/31640721866/job/94587529014) failed one test in `seatunnel-api`: ``` [ERROR] MultiTableSinkWriterSchemaChangeBroadcastTest.schemaChangeKeepsIOExceptionContractWhenWorkerAlreadyFailed Time elapsed: 0.326 s <<< ERROR! ``` I checked whether this test file is part of this PR's diff — it isn't (`git diff dev...pr10306 -- '**/MultiTableSinkWriterSchemaChangeBroadcastTest.java'` is empty); it's pre-existing on `dev`, added by #11726 ("Tolerate the pre-existing close() race in the schema-change worker-failure test"). But the class it exercises, `MultiTableWriterRunnable`, is one of the files this PR changes most heavily, so I don't think this is generic flakiness — I think it's a real interaction between this PR's changes and that test's assumptions. Concretely: - The failing test's own code comment (`MultiTableSinkWriterSchemaChangeBroadcastTest.java:412-417` on `dev`) says `close()` has *"a timing race in existing close() behavior... [the test tolerates] either outcome, but fail[s] if close() surfaces anything other than the exact failure already asserted."* That tolerance was written against the pending-flag mechanism that existed on `dev` at the time #11726 landed. - This PR replaces that exact mechanism. `MultiTableSinkWriter.write()` used to enqueue via the uncounted `MultiTableWriterRunnable.rowRequest(...)`; on this PR's head it enqueues via the new `writerRunnable.countedRowRequest(...)`, which increments a new `AtomicInteger pendingRowRequests`, decremented in a `finally` block in `MultiTableWriterRunnable.run()` after each queue element finishes. `close()`'s `checkQueueRemain()` path now branches on the new `writerRunnable.hasPendingRowRequests()` instead of whatever the old pending-flag read was (`MultiTableSinkWriter.java`, around the `checkQueueRemain()` call site). - In other words, the specific timing window the test's comment describes as "tolerate either outcome" was written against a mechanism this PR removes and replaces with a different one. It looks plausible that the new counter-based mechanism resolves that race deterministically into the branch the test does *not* tolerate — `close()` surfacing something other than the exact `rowWriteFailure` instance the test already asserted via `applySchemaChange(...)`. I want to be precise about what I have and haven't verified here: I traced this from the diff and the CI log, not by running the test myself (per this review's ground rules, I don't execute the PR locally) or attaching a debugger, so I can't hand you the exact line where the wrong exception gets constructed. But I'm confident enough in the mechanism-replacement finding above that I don't think this is an unrelated environment flake, and I'd treat it as blocking until it's either fixed or you can show it's a pre-existing/unrelated race (e.g. by pointing at a `dev`-only repro without this PR's changes). Two other jobs in the same run failed for what looks like a separate, unrelated reason: `Run / all-connectors-it-4 (11, ubuntu-latest)` (https://github.com/zhangshenghang/seatunnel/actions/runs/31640721866/job/94587529911) — I haven't dug into that one yet; worth checking whether it's the same root cause or something else entirely before assuming it's a single fix. **Revised state, replacing Issue 18 from my last review:** - Issue 18 is no longer "CI hasn't finished running" — it's now a real, likely PR-attributable test failure in `MultiTableSinkWriterSchemaChangeBroadcastTest#schemaChangeKeepsIOExceptionContractWhenWorkerAlreadyFailed`, plus a second failing job (`all-connectors-it-4`, 11/ubuntu) I haven't yet triaged. Severity: High — this sits directly in the close()/failure-surfacing contract of the exact class this PR rewrites, not a peripheral module. - @SEZ9's stale `CHANGES_REQUESTED` from `da827df05` does appear to be cleared now — `reviewDecision` on the PR is `APPROVED` as of this check, so that half of my last review's "before merge" list looks resolved. Everything else from my 2026-08-14 full re-review (Issues 5/6/7/10/14/15/16/17, and the rest of the runtime-path analysis) stands unchanged — this comment only updates the CI-gate item. Sorry for the back-and-forth on this one — I know CI has been an unusually long saga on this PR specifically because of the fork's queue starvation, and now that it's finally run, it surfaced something worth looking at rather than closing the loop. Happy to take another pass as soon as you've had a chance to look at the Windows unit-test failure and the all-connectors-it-4 job. -- 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]
