DanielLeens commented on PR #11834: URL: https://github.com/apache/seatunnel/pull/11834#issuecomment-5342948878
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 fresh, from-scratch pass over the current head (`56959a8e78`) — no new commit has landed since my last comment (2026-08-19 10:35 UTC), so the diff itself is unchanged from what I already analyzed in detail across my three prior comments. I re-verified the CI signal live rather than reusing the earlier read: fork run [32088668827](https://github.com/DanielLeens/seatunnel/actions/runs/32088668827) is still the only run recorded for this head_sha (no rerun has been triggered since my last comment), so the picture from that comment stands unchanged: `unit-test (8, windows-latest)` failed (`BUILD FAILURE`, 2 errors), `unit-test (11, windows-latest)` passed, and the two `ubuntu-latest` legs were cancelled alongside it. I don't want to repeat the full root-cause writeup from that comment, so this one focuses on (a) independently re-verifying the parts of the diff I hadn't personally re-derived line-by-line, and (b) a genuinely new angle I hadn't checked before: whether the CI evidence actually substantiates that the new `TestStallThreadDumpExtension` — the PR's own risk-mitigation story for "even if the flake isn't fully closed, at least it's now diagnosable" — actually produced anything in the one run where it mattered. # What Problem Does This PR Solve? `seatunnel-engine-server` unit tests intermittently fail on `windows-latest` CI with `IllegalStateException: Node failed to start!` after Hazelcast's ~300s join ceiling. The PR (a) replaces `localhost` with the concrete `127.0.0.1` literal everywhere this module's tests configure Hazelcast's `tcp-ip` member-list or construct `Address` objects, to remove Windows' dual-stack `localhost` → `[127.0.0.1, ::1]` resolution ambiguity that `AddressPicker` was tripping over; (b) adds `TestStallThreadDumpExtension`, a JUnit5 extension that is supposed to thread-dump a test class if its `@BeforeAll` bootstrap stalls past 240s, so a future stall is diagnosable instead of an opaque timeout; and (c) fixes a pre-existing `log4j2-test.properties` bug where the routing appender's `system` route pointed at an undefined `fileAppender`. # 1. Code Change Review ## 1.1 Core Logic Analysis **Loopback-IP substitution.** I independently re-traced the two claims from my earlier reviews that matter most for correctness rather than taking them on faith a third time: - `FollowerRunningJobsFilterTest` (one of the two classes that fails in the current CI run) does **not** override `getHazelcastConfig()` — I checked the file directly (`FollowerRunningJobsFilterTest.java:37-46`): it only *calls* the inherited `getHazelcastConfig()` at line 56 to boot a second "follower" node inside a test method. So it correctly inherits the fixed `127.0.0.1` literal from `AbstractSeaTunnelServerTest.java:89`, confirming the base-class-first fix does reach this class as claimed. - `SourceSplitEnumeratorTaskTest.java` (5 call sites, `Address.createUnresolvedAddress("localhost", ...)`, lines 115/165/220/319/320) is correctly left untouched — `createUnresolvedAddress` doesn't perform DNS resolution, unlike `Address(String,int)`, and this class is a pure-Mockito test with no real Hazelcast network layer. - 29 classes extend `AbstractSeaTunnelServerTest`; 8 override `getHazelcastConfig()` themselves (`WorkerTagTest`, `FollowerRunningJobsFilterTest` — wait, it doesn't, see above — `CoordinatorServiceWithCancelPendingJobTest`, `RestApiHttpsTest`, `RestApiHttpsForTruststoreTest`, `BaseServletTest`, `RestApiHttpBasicTest`, plus the base class itself). Only `WorkerTagTest` among the actual overriders is touched by this PR's diff; the REST/HTTPS overriders build plain HTTP(S) client URLs against `localhost`, not a Hazelcast member-list, so they're legitimately out of scope for this specific fix (as noted in Issue 1 of my 2026-08-17 comment). This part of the diff is sound and I have nothing to add beyond what's already on the thread. **`TestStallThreadDumpExtension` — did it actually do its job on the one run that mattered?** This is the part I hadn't checked yet. I pulled the raw failing-job log directly (`gh api repos/DanielLeens/seatunnel/actions/jobs/95566875398/logs`, 56,959 lines) rather than trusting the summarized excerpt from my previous comment, and grepped it end-to-end for any trace of this extension firing: ``` grep -in "stallthread\|has been inside its bootstrap\|seatunnel-test-stall-watchdog" job.log → 0 matches ``` `FollowerRunningJobsFilterTest` starts at `02:22:51.034` and fails at `02:28:02.223` — a 311s stall, which is 71s past `STALL_THRESHOLD_MILLIS` (240s) and comfortably past two `SCAN_PERIOD_MILLIS` (30s) polling cycles. The extension should have logged its `ERROR`-level dump somewhere around `02:26:51`–`02:27:21`. It didn't. Before concluding the extension itself is broken, I checked for a more mundane explanation: is *any* application-level log4j2 output visible in this job log at all? I grepped for the exact pattern `log4j2-test.properties` renders (`[%X{ST-JID}] %d ... %-5p [%-30.30c{1.}] [%t] - %m%n`) as well as looser variants, and for Hazelcast's own runtime log content (`AddressPicker`, cluster join messages, etc.) anywhere in the 339-test, ~50-minute engine-server run: ``` grep -cE '\] (WARN|ERROR) +\[' job.log → 0 grep -cE '^\[.*\] 202[0-9].* (WARN|ERROR) ' job.log → 0 grep -c "Hazelcast" job.log → 25, all Maven module/artifact names, zero runtime log content ``` Zero. Not just for the stalled window — for the entire module's test phase, at any log level, including the 337 *passing* tests that ran in the same JVM fork before and after the failure. The only continuous output visible during the stall is the raw, header-less `println`-style `SeaTunnelChildFirstClassLoader@66ba1bbc` spam (once/second) that my 2026-08-19 comment already flagged as a separate, pre-existing issue unrelated to this diff. I want to be precise about what this evidence does and doesn't prove: I can't be certain from the console capture alone whether the extension's `beforeAll`/watchdog never ran (autodetection failing to engage — plausible, since the extension's own Javadoc admits `junit-platform-launcher` isn't a project dependency here, though I'd note two *other* `junit-platform.properties` files already in this repo, e.g. `seatunnel-e2e-common`'s `junit.jupiter.testclass.order.default`, prove the general "properties-file-drives-Jupiter-engine-config" channel does work in this codebase for other `junit.jupiter.*` keys — so it's not a given that autodetection specifically is the failure point) versus it did run but its `log.error(...)` call was swallowed by the same log-capture gap that's swallowing every other application log line in this job. Either way, the observable fact is the same and is what matters for merge readiness: **in the one real CI run that stalled past this extension's own thr eshold, on this PR's own current head, nothing it was built to produce ever reached a place a human triaging CI could see it.** The PR's second stated deliverable — "so hangs like this become diagnosable from CI logs" — is unproven, and the one dataset available to test it says it didn't happen. ## 1.2 Compatibility Impact Fully compatible. All 16 changed files are under `src/test/java` or `src/test/resources` in `seatunnel-engine-server`; `git diff dev...HEAD --stat` confirms zero touches to `src/main/**`. No production runtime, API, config, or serialization impact. ## 1.3 Performance / Side-Effect Analysis Negligible for the IP-literal substitutions. The watchdog is a single daemon thread per JVM fork, waking every 30s to scan a small map — trivial cost, and being a daemon thread it can't block surefire JVM shutdown. No new resource-leak or concurrency-safety concern in the extension's own code (CAS-guarded single-start, `ConcurrentHashMap`/`ConcurrentHashMap.newKeySet()` for cross-thread map access between JUnit's callback-invoking thread and the watchdog thread). ## 1.4 Error Handling and Logging The watchdog's `InterruptedException` handling correctly restores the interrupt flag and returns rather than swallowing it (`TestStallThreadDumpExtension.java` `scanLoop()`). No sensitive data is logged. See Issue 4 below for the logging-visibility gap. # 2. Code Quality Assessment ## 2.1 Coding Standards Clean; the extension is well-commented on the "why" (240s vs. Hazelcast's 300s ceiling, why the window is bounded to `beforeAll`→`beforeEach`, why service-loader autodetection was chosen). No missing-doc-comment issues on the new class's public/package-visible members. ## 2.2 Test Coverage and Test Stability **Stability rating: High risk.** This is a revision of my own 2026-08-19 rating (I'd already moved it from "Stable" to "Risk present" once the current-head CI run reproduced the target `Node failed to start!` failure in files this PR touches — see Issue 3, still standing). I'm moving it further to **High risk** based on the new evidence in 1.1: the diff's own diagnostic safety-net for exactly this failure mode does not demonstrably work, in the one run that would have proven it. A "High risk" rating requires a formal High-severity issue per the review checklist — see Issue 4 below. This is on top of, not a replacement for, Issue 3. To be fair to the parts that do hold up: the `127.0.0.1` substitutions remain deterministic, side-effect-free literal edits with no new timing assumption, and I don't have evidence contradicting my earlier read that they close the specific dual-stack-resolution mechanism from the originally-cited CI log. The risk is concentrated in (i) the underlying flake only being partially closed (Issue 3) and (ii) the new diagnostic tooling meant to make future occurrences triageable not being proven to work (Issue 4). ## 2.3 Documentation Updates None required — test-only, zero user-facing behavior. # 3. Architectural Soundness ## 3.1 Elegance Precise fix for the loopback-IP substitution itself (base-class-first, evidenced against a real CI log). The stall-dump extension is a reasonable idea executed with good intent, but per 1.1/2.2 its actual effectiveness in this codebase's Windows CI environment is unverified — I'd classify it as a well-intentioned addition whose value is not yet proven, not a "long-term solution" yet. ## 3.2 Maintainability Good structure and comments, but see Issue 4 — a diagnostic tool that can't be shown to produce output is a maintainability trap in its own right: the next person who hits this flake and doesn't see a dump will reasonably (and wrongly) assume the extension isn't registered at all, when the truth might just be a log-capture gap. ## 3.3 Extensibility Unchanged from my 2026-08-18 note: if this stall pattern recurs elsewhere, the extension would be worth promoting to a shared test-support artifact — not blocking for this PR. ## 3.4 Historical-Version Compatibility N/A — no serialization, checkpoint/savepoint, protocol, or config-option surface touched. # 4. Issue Summary | Number | Issue | Location | Severity | |---|---|---|---| | Issue 3 | Current head's own Windows CI run reproduces the exact `Node failed to start!` failure this PR targets, in two files it modified (`FollowerRunningJobsFilterTest` via inherited `AbstractSeaTunnelServerTest.java:76-89`, and `RestApiSubmitJobStartWithSavePointTest.java:97,523`) | fork run [32088668827](https://github.com/DanielLeens/seatunnel/actions/runs/32088668827) job [95566875398](https://github.com/DanielLeens/seatunnel/actions/runs/32088668827/job/95566875398) | High | | Issue 4 (new) | `TestStallThreadDumpExtension`'s diagnostic output cannot be found anywhere in the one CI run that stalled past its 240s threshold (311s actual stall, two full 30s scan cycles of margin); more broadly, this job's entire engine-server test phase (339 tests, ~50 min) shows zero log4j2-formatted application/Hazelcast log output at any level, so neither "the extension fired" nor "the extension is correctly registered but silent" can be distinguished from the available evidence — the PR's own "diagnosable from CI logs" claim is unverified in practice | `seatunnel-engine/seatunnel-engine-server/src/test/java/org/apache/seatunnel/engine/server/TestStallThreadDumpExtension.java` (whole file); `.../src/test/resources/junit-platform.properties`; `.../src/test/resources/log4j2-test.properties` | High | | Issue 1 | PR title/description scope has drifted from the current diff (loopback-IP title, but diff also adds the stall-dump extension and an unrelated log4j2 fix) | PR title/description vs. current diff | Low | | Issue 2 | No dedicated unit test for `TestStallThreadDumpExtension`'s own map add/remove/dump logic | `TestStallThreadDumpExtension.java` (whole file, no companion test) | Low | No new correctness issues found in the loopback-IP substitution itself beyond what's already on this thread. # 5. Merge Recommendation ### Conclusion: Not recommended for merge 1. **Blockers — must be fixed** - Issue 3: the target failure reproduces on this PR's own current head. Either get a clean rerun and treat this occurrence as noise if it doesn't reproduce, or root-cause why `RestApiSubmitJobStartWithSavePointTest` — which is unambiguously on `127.0.0.1`, no hostname involved — still fails to join within 300s (thread starvation, port exhaustion from `port-count: 100` colliding across the module's many classes, or the leaked `SeaTunnelChildFirstClassLoader` background print are all more likely candidates than DNS/dual-stack at this point). - Issue 4: before relying on `TestStallThreadDumpExtension` as this PR's risk-mitigation story, prove it actually produces visible output on a real stall in this CI environment. Concretely: add a one-line unconditional `log.info(...)` (or even `System.out.println`, given that channel is demonstrably captured) inside `beforeAll()` itself — independent of the 240s wait — as a canary, push, and confirm it shows up in a fresh Windows run; then confirm the actual `ERROR` dump line shows up too, either by lowering the threshold temporarily to force a fire or by waiting for a natural reproduction. If the canary line itself doesn't show up, that's strong evidence autodetection isn't engaging in this module's Surefire/JDK-8/Windows combination and the extension needs a different registration mechanism (e.g. an explicit `-Djunit.jupiter.extensions.autodetection.enabled=true` in the module's surefire `systemPropertyVariables`, or a `@RegisterExtension`/`@ExtendWith` per base class ins tead of relying on service-loader discovery). 2. **Recommended fixes — non-blocking** - Issue 1 and Issue 2 from my earlier comments still stand as low-severity nice-to-haves. Overall: the loopback-IP substitution itself is sound, evidenced, and I have no new objection to it after re-checking the subclass hierarchy and the `createUnresolvedAddress` near-miss myself. But the PR as a whole is making two claims — "this closes the flake" and "if it doesn't, you'll at least be able to diagnose it" — and the one CI run available to test both claims contradicts the first and can't substantiate the second. I'd rather hold this open one more round and get positive evidence on Issue 4 (plus a clean or well-understood rerun on Issue 3) than merge on the strength of the reasoning alone, given this is exactly the kind of Windows-CI-stability work where "looks right on paper" has already cost multiple rounds on this same PR. -- 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]
