DanielLeens commented on PR #11834: URL: https://github.com/apache/seatunnel/pull/11834#issuecomment-5340930009
Since this is my own PR, GitHub still won't let me submit a formal review state on it, so this is posted as a plain comment again. This is a follow-up to my most recent review (2026-08-18 01:52 UTC) on this exact head (`56959a8e78`) — no new commit has landed since then. In that review I explicitly flagged one open item: "The run for the current head (`56959a8e78`) was still queued/in progress as of this review — worth re-checking once it completes." That run has since finished, and the result changes my conclusion, so I want to correct the record rather than let a stale "Stable" verdict stand. # What I Found Re-Checking the Completed CI Run `unit-test (8, windows-latest)` on the current head (fork run [32088668827](https://github.com/DanielLeens/seatunnel/actions/runs/32088668827), job [95566875398](https://github.com/DanielLeens/seatunnel/actions/runs/32088668827/job/95566875398)) **failed** with `BUILD FAILURE`, `Tests run: 339, Failures: 0, Errors: 2`: ``` [ERROR] FollowerRunningJobsFilterTest>AbstractSeaTunnelServerTest.before:70 » IllegalState [ERROR] RestApiSubmitJobStartWithSavePointTest.setUp:97 » IllegalState Node failed to ... ``` Both errors are `java.lang.IllegalStateException: Node failed to start!` — the exact symptom this PR exists to eliminate — and both fire in files this PR itself touches: - `FollowerRunningJobsFilterTest` (`seatunnel-engine/seatunnel-engine-server/src/test/java/org/apache/seatunnel/engine/server/master/FollowerRunningJobsFilterTest.java:38,56`) does not override `getHazelcastConfig()`, so it inherits the fixed base config from `AbstractSeaTunnelServerTest.java:76-89`, which now uses `127.0.0.1` (`AbstractSeaTunnelServerTest.java:89`). - `RestApiSubmitJobStartWithSavePointTest` (`.../rest/RestApiSubmitJobStartWithSavePointTest.java:97,523`) has its own inline Hazelcast config, and this PR's own diff already changed its `member-list` entry from `localhost` to `127.0.0.1` at line 523. So this is not a case of the fix missing a spot — both failing classes are already on the `127.0.0.1` literal introduced by this PR, and the join still failed after ~311s (`hazelcast.max.join.seconds` + shutdown overhead), on the current head, in a fork CI run that ran to completion. I also compared this against `unit-test (11, windows-latest)` on the same head (job [95566875283](https://github.com/DanielLeens/seatunnel/actions/runs/32088668827/job/95566875283)), which passed (`BUILD SUCCESS`). So the picture is: same commit, same module, one JDK matrix leg green, one red — consistent with residual non-determinism rather than a deterministic break, but it does mean the loopback substitution has **not** been empirically shown to fully close out the flake it targets. One more thing worth flagging while I was in the log: starting partway through `CoordinatorServiceTest` and continuing across many unrelated later test classes (`PendingDiagnosticsCollectorTest`, `JobEventHttpReportHandlerTest`, and on through the `rest.*` classes), the log shows a bare, header-less line repeating roughly once per second: ``` org.apache.seatunnel.engine.common.loader.SeaTunnelChildFirstClassLoader@66ba1bbc ``` 948 occurrences in the failing job's log, 227 in the passing JDK-11 job's log for the same head — so it's present in both, it's not new in this PR's diff (I checked; it isn't emitted anywhere in the 16 changed files, and the new `TestStallThreadDumpExtension` only fires once per stalled class at `ERROR` level with a fully-formatted multi-line dump — see `TestStallThreadDumpExtension.java:124-133` — it cannot produce this pattern), and since it shows up in a passing run too I don't think it's the direct cause of the join failures. But a background thread printing a raw classloader identity every second for 15+ minutes across unrelated test classes, with no logger header at all, points at some pre-existing leaked/undying thread in this module's test suite that nobody has traced yet. I'm not attributing today's failures to it — I don't have enough evidence for that — but it's a loose end that makes we more cautious about declaring the module's Windows CI "stabilized," and i t's a separate, standing bug independent of this PR's scope. # Revised Conclusions ## 2.2 Test Coverage and Test Stability — revised **Stability rating: Risk present** (revised down from my previous "Stable"). To be precise about what changed and why: the substitutions themselves (`localhost` → `127.0.0.1`) remain sound, deterministic, side-effect-free literal edits — I stand by that part of the previous analysis, and they are very likely a real, partial improvement (the specific dual-stack `AddressPicker` resolving-to-two-addresses signature from the originally-cited CI log does not appear anywhere near either failure in this run, so that particular mechanism does look closed). What I got wrong last time was asserting "Stable" before the one CI signal that could actually falsify the claim — the current head's own Windows run — had finished. Now that it has, and it reproduces the target exception in two files this PR modified, I can't call this "Stable" with a straight face. It isn't "High risk" either, since the diff doesn't add any new anti-pattern (no `Thread.sleep`, no new timing assumption, nothing non-deterministic introduced) — the risk is that the underlying flake this PR targets is only partially closed, not that the PR makes anything worse. ## Issue 3 (new) — CI on the current head reproduces the exact target failure in files this PR modified - **Location:** `seatunnel-engine/seatunnel-engine-server/src/test/java/org/apache/seatunnel/engine/server/master/FollowerRunningJobsFilterTest.java` (inherits `AbstractSeaTunnelServerTest.java:76-89`) and `.../rest/RestApiSubmitJobStartWithSavePointTest.java:97,523`; evidence in fork run [32088668827](https://github.com/DanielLeens/seatunnel/actions/runs/32088668827) job [95566875398](https://github.com/DanielLeens/seatunnel/actions/runs/32088668827/job/95566875398). - **Problem description:** Both classes already carry this PR's `127.0.0.1` fix, yet both hit `IllegalStateException: Node failed to start!` after the same ~311s join ceiling that motivated this PR, on the PR's own current head. - **Potential risk:** Merging on the premise "this closes the Windows `Node failed to start!` flake" would be inaccurate — CI evidence from this exact head says the flake is reduced at best, not eliminated. If merged as-is without flagging this, the next occurrence on `dev` will look like a brand-new regression to whoever triages it, when it's actually a known-incomplete fix. - **Best improvement:** Before merging, either (a) get a clean rerun of `unit-test (8, windows-latest)` and treat this occurrence as noise if it doesn't reproduce, explicitly noting in the PR description that the fix reduces but may not fully eliminate the flake, or (b) dig into why `RestApiSubmitJobStartWithSavePointTest` — which is now unambiguously on `127.0.0.1`, no hostname involved at all — still fails to join within 300s, since that rules out DNS/dual-stack as the cause for *this* occurrence and points at something else (thread starvation, port exhaustion from `port-count: 100` + `auto-increment` colliding across the many classes in this suite, or the leaked background thread noted above are all more likely candidates worth checking before claiming victory). - **Severity:** High - **Raised by another reviewer:** No. # 5. Merge Recommendation ### Conclusion: Not recommended for merge (pending CI triage) 1. **Blockers — must be fixed** - Issue 3 above: the current head's own Windows CI run reproduces the exact `Node failed to start!` failure this PR is meant to fix, in two files the PR itself modified. This needs to be re-run and, if it reproduces again, actually root-caused (or at minimum the PR description needs to stop claiming this eliminates the flake) before merge. 2. **Recommended fixes — non-blocking** - The title/description-scope drift and missing unit test for `TestStallThreadDumpExtension` from my previous review still stand as low-severity, non-blocking nice-to-haves. - The repeating `SeaTunnelChildFirstClassLoader@...` background print noted above is worth a separate investigation (not this PR's scope, and not proven to be the cause of the join failures), since a per-second print for 15+ minutes across unrelated test classes on every Windows run is its own smell. I want to be fair to the work here: the loopback-IP substitution is very likely a genuine partial fix, and the new stall-dump extension and log4j2 fix are both clean, safe, well-reasoned additions — none of that is undone by this finding. What's undone is the previous round's premature "Stable"/"root-cause fix" framing, which I gave before the one piece of evidence that could contradict it had actually landed. Apologies for asserting that conclusion a beat too early — I should have waited for the run to finish before rating it. Given this is my own PR, I'd like to actually re-run the Windows JDK 8 job a couple more times to see if `RestApiSubmitJobStartWithSavePointTest` reproduces, before asking anyone to spend review time on this again. -- 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]
