DanielLeens commented on PR #11834:
URL: https://github.com/apache/seatunnel/pull/11834#issuecomment-5379284867
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 my seventh
pass on this PR. The head commit (`56959a8e78`) is unchanged since my last
comment, so I did not re-derive the code analysis from zero a seventh time — I
spot-checked the load-bearing claims from prior rounds directly against the
current tree rather than trusting my own summaries, and I re-pulled the CI
evidence from raw job logs myself rather than reusing the prior round's
write-up. Short version: the verdict is holding steady this time, not flipping
again. Everything below is what I independently re-verified today, not a
restatement of the prior round.
# 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
~300-second join ceiling. This 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, removing Windows'
dual-stack `localhost -> [127.0.0.1, ::1]` resolution ambiguity that
`AddressPicker` was tripping over; (b) adds `TestStallThreadDumpExtension`, a
JUnit 5 extension that logs a full thread dump if a test class's bootstrap
stalls past 240 seconds, 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`, which
was silently spamming "Appender fileAppender cannot be located" on every
bootstrap. All 16 changed files are confined to `src/test/java` and
`src/test/resources` in
`seatunnel-engine-server`.
# 1. Code Change Review
## 1.1 Core Logic Analysis
I re-verified the following directly against the current tree (`git
grep`/`git show` on the worktree HEAD, not trusting the earlier write-ups):
- `git grep -l "extends AbstractSeaTunnelServerTest"` returns exactly 29
classes. `git grep -n "String getHazelcastConfig"` shows the method is
overridden in exactly 4 places (`WorkerTagTest`, `OptionRulesApiTest`,
`RestApiSubmitJobConfigShadeDecryptTest`,
`RestApiSubmitJobStartWithSavePointTest`) besides the base class — and those 4
are exactly the files this PR also edits individually. Every other one of the
29 subclasses, including the classes previously named as flaky (`TaskTest`,
`CoordinatorServicePipelineCleanupTest`, `FollowerRunningJobsFilterTest`),
inherits the fixed `127.0.0.1` config from
`AbstractSeaTunnelServerTest.getHazelcastConfig()`
(`AbstractSeaTunnelServerTest.java:76-89`) automatically. The base-class-first
fix is confirmed complete for the module.
- `SourceSplitEnumeratorTaskTest.java` still calls
`Address.createUnresolvedAddress("localhost", ...)` in 5 places.
`createUnresolvedAddress` never resolves DNS (unlike the `Address(String, int)`
constructor used elsewhere in this diff, which is why `generateWorker` methods
declare `throws UnknownHostException`), so leaving these untouched is correct,
not a missed spot.
- `TestStallThreadDumpExtension.java`: lifecycle is sound — `beforeAll`
records a start timestamp per test-class name in a `ConcurrentHashMap`;
`beforeEach` and `afterAll` both clear it, so a class's own long-running test
methods can't trigger a spurious dump, and a class whose `@BeforeAll` never
completes still gets cleaned up via `afterAll` after the failure propagates.
The single watchdog daemon thread is CAS-guarded
(`WATCHDOG_STARTED.compareAndSet`) so it starts at most once per JVM, and
`InterruptedException` in `scanLoop()` correctly restores the interrupt flag
and returns rather than swallowing it.
- Scope confinement: I checked whether this extension could leak into other
modules via a test-jar. `seatunnel-engine-server/pom.xml` declares one
`test-jar`-typed dependency, and it's on `seatunnel-e2e-common` (i.e. this
module *consumes* another module's test-jar) — I confirmed there is no
`maven-jar-plugin` `test-jar` goal bound in this module's own `pom.xml`, so
this module does not itself produce a test-jar artifact, and nothing else in
the repo declares a `test-jar` dependency on `seatunnel-engine-server`. The
extension's `junit-platform.properties` autodetection is correctly confined to
this module's own test run.
- `log4j2-test.properties`: the new `appender.file.*` block mirrors
`config/log4j2.properties` and resolves against
`file_path`/`file_name`/`file_split_size` properties already declared earlier
in the same file. Legitimate, narrowly-scoped fix.
- License header: the new
`META-INF/services/org.junit.jupiter.api.extension.Extension` file carries the
full ASF header as of the current head — confirmed by reading the file directly.
**Runtime path** (test-bootstrap only, does not touch `src/main/**`):
```
AbstractSeaTunnelServerTest.before() [JUnit @BeforeEach in the shared base
class]
-> builds Hazelcast Config from getHazelcastConfig() YAML string
-> HazelcastInstanceFactory.newHazelcastInstance(config)
-> TcpIpJoiner reads config's tcp-ip member-list ("127.0.0.1" after this
PR)
-> AddressPicker.getPossibleSocketAddresses() calls
InetAddress.getAllByName() on each entry
(this is exactly where "localhost" used to resolve to both 127.0.0.1
and ::1 on Windows)
-> node joins itself / other in-process members using the resolved Address
```
`TestStallThreadDumpExtension` runs alongside this as a JUnit extension hook
(`beforeAll`/`beforeEach`/`afterAll`), outside this call chain — it only
observes wall-clock elapsed time, it does not participate in the join itself.
## 1.2 Compatibility Impact
Fully compatible. `git diff dev...HEAD --stat` confirms zero files under
`src/main/**`; every changed file is `src/test/java` or `src/test/resources` in
`seatunnel-engine-server`. No production runtime, API, config, or serialization
impact.
## 1.3 Performance / Side-Effect Analysis
Negligible. The IP substitutions are pure string-literal swaps. The watchdog
is a single daemon thread per JVM fork, sleeping 30s between scans of a small
in-memory map — trivial CPU/memory, and being a daemon thread it cannot block
Surefire JVM shutdown even if the scan loop is still sleeping when the JVM
exits.
## 1.4 Error Handling and Logging
`InterruptedException` handling in the watchdog is correct (flag restored,
not swallowed). The stall-dump log is `ERROR`-level, appropriate since it
signals a previously-invisible, actionable failure mode, and it only logs
thread/lock names and stack frames — no argument values, no credentials. See
Issue 2 below for the one still-open point on this extension.
# 2. Code Quality Assessment
## 2.1 Coding Standards
Clean. `TestStallThreadDumpExtension` is unusually well-commented for
test-support code — it explains why the 240s threshold sits below Hazelcast's
300s join ceiling, why the monitored window is `beforeAll`->`beforeEach` and
not the whole class lifecycle, and why service-loader autodetection was chosen
over a launcher `TestExecutionListener` (avoiding a `junit-platform-launcher`
pom addition). No missing-doc-comment issues on the new class or its fields.
## 2.2 Test Coverage and Test Stability
**Stability rating: Risk present** (holding steady from my last round, not
re-upgrading to "Stable" yet — see reasoning below).
I re-pulled the live CI evidence myself today rather than reusing the prior
round's summary:
- `gh api repos/DanielLeens/seatunnel/commits/56959a8e78.../check-runs`
(current head, no newer run exists for this SHA — `gh pr view --json
headRefOid` confirms the head is still `56959a8e78`, last commit date unchanged
at 2026-08-18T01:33:41Z) shows `Run / unit-test (8, windows-latest)` and `Run /
unit-test (11, windows-latest)` both `conclusion: success`, along with both
Ubuntu unit-test legs.
- Across the full 87-check-run set for this head, the only non-success,
non-skipped entry is `Run / all-connectors-it-6 (8, ubuntu-latest)`. I pulled
that job's raw log directly (job id 96298440772) rather than trusting the
description: the actual failure is
`org.awaitility.core.ConditionTimeoutException: ... expected: <1> but was: <0>
within 2 minutes` in
`PostgresCDCIT.testPostgresCdcSnapshotOnlyAndCommittedOffsetStartupModes`,
inside `connector-cdc-postgres-e2e` — a module this PR's 16-file diff never
touches. Independently confirmed unrelated.
Why "Risk present" and not "Stable": this is still the same single clean CI
run examined more carefully, not a second independent rerun — I have one data
point (this run) showing the Windows jobs green, and one earlier data point
(the 08-19 finding) showing them red on an earlier attempt of the same head.
One clean result after one red result on a genuinely intermittent flake is
encouraging, not dispositive. Separately, `TestStallThreadDumpExtension`'s own
effectiveness remains unverified either way — neither Windows job in this run
stalled past 240s, so there was nothing for the extension to prove itself
against. That's unchanged from last round and still an open item, not a blocker
(see Issue 2).
## 2.3 Documentation Updates
None required — test-only, zero user-facing behavior change.
# 3. Architectural Soundness
## 3.1 Elegance of the Solution
Precise fix. Fixing the shared base class first, rather than patching every
failing subclass, is the correct leverage point, and it is evidenced against a
real, previously-captured CI log showing `AddressPicker` resolving `localhost`
to two addresses immediately before a join failure. The stall-dump extension is
a sound, well-scoped diagnostic addition; its practical payoff in this specific
CI environment (JDK 8/11, Windows, Surefire) is still unproven per 2.2, so I'd
call it "sound idea, unverified payoff" rather than fully validated.
## 3.2 Maintainability
Good. The one standing maintainability risk is that a diagnostic tool nobody
has seen actually fire could be silently non-functional (e.g., wrong logger
configuration suppressing its output in this CI environment) and nobody would
notice until the next real stall.
## 3.3 Extensibility
Non-blocking observation, unchanged from prior rounds: if this stall pattern
recurs in other Hazelcast-based test modules, `TestStallThreadDumpExtension`
would be worth promoting into shared test-support code. Not this PR's scope.
## 3.4 Historical-Version Compatibility
N/A — no serialization, checkpoint/savepoint, protocol, or config-option
surface is touched.
# 4. Issue Summary
| # | Issue | Location | Severity |
|---|---|---|---|
| 1 | PR title/description still reads "Use loopback IP in engine server
tests," but the current head also adds the stall-dump extension and an
unrelated log4j2 fix | PR title/description vs. current diff | Low |
| 2 | `TestStallThreadDumpExtension`'s diagnostic output has still not been
confirmed to actually appear in this CI environment on a real stall — no run
examined so far (including today's) has stalled past 240s, so this remains
genuinely unverified rather than resolved | `TestStallThreadDumpExtension.java`
(whole file); `junit-platform.properties`; `log4j2-test.properties` | Medium |
| 3 | 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 this round. No non-Daniel reviewer has
commented on this PR yet (checked the full `reviews`/`comments`/`reviewThreads`
history) — there is nothing to reconcile against.
# 5. Merge Recommendation
### Conclusion: Ready to merge after fixes
1. **Blockers — must be fixed:** None. The code-level analysis has now held
up across seven independent passes with no remaining correctness objection, and
today's CI re-verification (pulled from raw job logs, not reused summaries)
confirms both Windows unit-test legs are green on the unchanged current head,
with the sole remaining CI failure independently confirmed unrelated (Postgres
CDC IT timeout in a module this PR never touches).
2. **Recommended fixes — non-blocking:**
- Issue 2: add an unconditional canary `log.info(...)` in `beforeAll()`
(independent of the 240s wait) to confirm this extension's output actually
reaches CI logs in this Surefire/JDK-8/Windows combination — this can't make
the current fix worse, but it would close the one genuinely open question about
whether the diagnostic tooling works at all.
- Issue 1 (title/description scope) and Issue 3 (extension unit test)
remain low-severity nice-to-haves.
Overall: the substance of this PR — the loopback-IP substitution as a
root-cause fix, backed by a real CI log showing the exact dual-stack resolution
mechanism, plus a safe-by-construction diagnostic extension and an unrelated
but legitimate log4j2 bug fix — has not changed across seven rounds of
scrutiny, only the confidence in the CI evidence has moved (down once when a
same-head rerun reproduced the target flake, back up when a later rerun of that
same head came back clean). I'm not going to keep re-running my own CI to chase
a fully "Stable" rating on an admittedly intermittent flake; the current
evidence is good enough to merge with the two non-blocking follow-ups above
tracked, and I'd like a maintainer to take this over for the actual merge
decision now that the evidence has held steady for two consecutive checks on
the same head.
--
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]
