DanielLeens commented on PR #11834:
URL: https://github.com/apache/seatunnel/pull/11834#issuecomment-5312638959
Since this is my own PR, this is posted as a comment rather than a formal
review.
# What Problem Does This PR Solve?
`seatunnel-engine-server` unit tests intermittently fail on `windows-latest`
CI with `IllegalStateException: Node failed to start!` (and related Hazelcast
cluster-join/cleanup timeouts). The root cause is that Hazelcast's
`AddressPicker`, when a test's `tcp-ip` `member-list` is configured as the
hostname `localhost`, resolves it to *both* `127.0.0.1` and `0:0:0:0:0:0:0:1`
on Windows runners, and can pick different addresses for different members of
the same in-process cluster — this PR's own referenced CI log for `TaskTest`
shows `AddressPicker` logging "Resolving domain name 'localhost' to
address(es): [127.0.0.1, 0:0:0:0:0:0:0:1]" immediately before the node fails to
join itself. Replacing the hostname with the concrete IPv4 loopback literal
`127.0.0.1` removes the ambiguity at its source. This PR replaces `localhost`
with `127.0.0.1` across the Hazelcast test configs
(`AbstractSeaTunnelServerTest`, `WorkerTagTest`, `OptionRulesApiTest`, two REST
tests, `hazelcast.yaml`, `haze
lcast-client.yaml`) and the test-only `Address`/worker construction sites in
the resource-manager tests, all confined to `src/test/**`.
# 1. Code Change Review
## 1.1 Core Logic Analysis
Two distinct classes of change, both correctly targeted:
- **Hazelcast `tcp-ip` `member-list` config**
(`AbstractSeaTunnelServerTest.java:17`, `WorkerTagTest.java`,
`OptionRulesApiTest.java`, `RestApiSubmitJobConfigShadeDecryptTest.java`,
`RestApiSubmitJobStartWithSavePointTest.java`, `hazelcast.yaml`,
`hazelcast-client.yaml`) — this is exactly the string `AddressPicker` resolves
via `InetAddress.getAllByName`, and is the direct trigger of the observed
dual-stack ambiguity. Fixing it at the single shared base class
(`AbstractSeaTunnelServerTest.getHazelcastConfig()`) is the highest-leverage
change in the diff: any subclass that does not override `getHazelcastConfig()`
inherits the fix automatically. I traced the full subclass hierarchy (28
classes extend `AbstractSeaTunnelServerTest`) and confirmed both cited-as-flaky
classes, `TaskTest` and `CoordinatorServicePipelineCleanupTest`, do not
override `getHazelcastConfig()` — they pick up the base-class fix without
needing their own file changes.
- **`Address("localhost", port)` construction in resource-manager tests**
(`FakeResourceManager.java`,
`FakeResourceManagerForExternalExceptionTest.java`,
`FakeResourceManagerForRequestSlotRetryTest.java`, `ResourceManagerTest.java`)
— Hazelcast's `Address(String, int)` constructor eagerly resolves the hostname
via `InetAddress.getByName` (hence the `throws UnknownHostException` on
`generateWorker`), so this is a second, independent place the same dual-stack
resolution could occur, correctly caught.
I specifically checked for a plausible third category the PR could have
wrongly touched or wrongly skipped: `SourceSplitEnumeratorTaskTest.java` still
uses `Address.createUnresolvedAddress("localhost", 5701)` in five places, which
at first glance looks like a missed instance of the exact pattern fixed
elsewhere. On inspection, `createUnresolvedAddress` (unlike the
`Address(String, int)` constructor) does not perform DNS resolution — it stores
the hostname as an opaque identifier for a pure-Mockito unit test that never
boots a real Hazelcast network layer. Leaving it untouched is correct, not an
oversight.
## 1.2 Compatibility Impact
All 12 changed files are under `src/test/java` or `src/test/resources`; no
file under `src/main/java` is touched. Zero production runtime impact, matching
the PR description's own claim.
## 1.3 Performance / Side-Effect Analysis
None — string literal substitutions in test bootstrap config and address
construction; no algorithmic or resource-usage change.
## 1.4 Error Handling and Logging
No error-handling or logging code is touched. The `throws
UnknownHostException` signatures on the `generateWorker` methods are unaffected
by the literal change (a numeric-IP string still goes through the same
constructor path).
# 2. Code Quality Assessment
## 2.1 Coding Standards
Straightforward literal substitutions plus one added explanatory comment
(`AbstractSeaTunnelServerTest.java:77`); consistent with the surrounding style.
Two incidental trailing-newline fixes (`batch_fake_to_inmemory.conf`,
`hazelcast.yaml`) are harmless hygiene improvements bundled into the same lines
that were already being touched.
## 2.2 Test Coverage and Test Stability
Evaluated against the flaky-test-pattern checklist:
- **Real, previously-observed failure vs. hypothetical**: Confirmed directly
from the cited CI run (`abolfazlmadanii/seatunnel` run `31946366223`, attempt
1) — job `unit-test (11, windows-latest)` failed with `TaskTest` erroring after
311s with `java.lang.IllegalStateException: Node failed to start!`, preceded in
the log by `AddressPicker` resolving `localhost` to both an IPv4 and an IPv6
loopback address and a same-cluster connection being closed with reason
"Connecting to self!" — a textbook symptom of a node picking the "wrong" one of
two equally-valid loopback addresses relative to its peer. This matches a
failure pattern already tracked separately (three prior independent sightings
across different test classes on `windows-latest`).
- **Root-cause fix vs. band-aid**: This is a root-cause fix, not a timing
band-aid — no `Thread.sleep`, no retry/backoff added anywhere in the diff. It
removes the actual ambiguous input (`localhost`) that Hazelcast's address
resolution was tripping over, rather than papering over the race with a wait.
- **Completeness**: The Hazelcast member-list and `Address(String,int)`
construction sites are fully covered — no remaining `member-list: localhost`
(or equivalent) exists anywhere in `seatunnel-engine-server`'s test or main
resources after this change. One category is knowingly left out of scope: plain
HTTP/HTTPS client URLs of the form `http://localhost:PORT` /
`https://localhost:PORT` still exist in several `AbstractSeaTunnelServerTest`
subclasses used to hit each test's own embedded REST endpoint (e.g.
`RestApiHttpsTest.java`, `BaseServletTest.java`,
`RestApiHttpsForTruststoreTest.java`, `RestApiHttpBasicTest.java`,
`MetricsApiTest.java`, and the REST-call lines in
`OptionRulesApiTest.java`/`RestApiSubmitJobStartWithSavePointTest.java`/`RestApiSubmitJobConfigShadeDecryptTest.java`
that weren't already covered by the member-list fix). These go through
`java.net.URL`/`HttpURLConnection`, a different code path from Hazelcast's
`TcpIpJoiner`/`AddressPicker`, and none of them app
ear in the cited failure evidence, so leaving them out of scope for a fix
aimed specifically at the Hazelcast cluster-join flake is reasonable. It is
still the same underlying dual-stack-resolution risk class, so it is worth
flagging as a residual item rather than silently declaring the module fully
immunized.
- **Risk of the fix itself flaking**: None identified — `127.0.0.1` is a
static, unambiguous literal; no new timing dependency or shared-state risk is
introduced.
- **Rating: Risk present (not High)** — the core mechanism is sound and
evidenced; the residual REST-URL `localhost` usages are the only open item, and
they are outside this PR's stated and evidenced scope.
## 2.3 Documentation Updates
None required — internal test infrastructure change with no user-facing
behavior.
# 3. Architectural Soundness
## 3.1 Elegance of the Solution
Fixing the shared base class
(`AbstractSeaTunnelServerTest.getHazelcastConfig()`) rather than patching every
failing subclass individually is the right leverage point, and the diff
correctly identifies which subclasses need their own additional fix (because
they override the method with their own copy of the YAML) versus which ones
need none (because they inherit it).
## 3.2 Maintainability
Small, mechanical, easy-to-audit diff. The one added comment explains the
"why" at the point closest to the actual fix.
## 3.3 Extensibility
N/A for a test-stability literal fix.
## 3.4 Historical-Version Compatibility
N/A — test-only, no state/serialization/checkpoint format involved, no
production runtime path touched.
# 4. Issue Summary
| Number | Issue | Location | Severity |
|---|---|---|---|
| Issue 1 | HTTP/HTTPS client URLs still use `localhost` (a different code
path than the Hazelcast member-list/Address fix applied elsewhere in this PR);
same dual-stack-resolution risk class but out of this PR's evidenced scope |
`RestApiHttpsTest.java`, `BaseServletTest.java`,
`RestApiHttpsForTruststoreTest.java`, `RestApiHttpBasicTest.java`,
`MetricsApiTest.java`, and remaining REST-call sites in
`OptionRulesApiTest.java` / `RestApiSubmitJobStartWithSavePointTest.java` /
`RestApiSubmitJobConfigShadeDecryptTest.java` | Low |
# 5. Merge Recommendation
### Conclusion: Ready to merge after fixes
1. **Blockers — must be fixed**
- None. The change is test-only, the root-cause reasoning is verified
against the actual failing CI log rather than assumed, and the fix's coverage
of the Hazelcast member-list/Address construction paths is complete for the
module.
2. **Recommended fixes — non-blocking**
- Issue 1: if Windows CI is later observed to flake on any of the
REST-endpoint tests with a similar dual-stack signature, apply the same
`127.0.0.1` substitution to their `http(s)://localhost:PORT` client URLs. Not
worth doing speculatively without evidence, consistent with keeping this PR's
diff minimal and targeted at the specific failures it cites.
Overall assessment: this is a minimal, well-targeted, evidence-backed
test-stability fix. I verified the claimed root cause directly against the
referenced CI run's logs (dual-stack `AddressPicker` resolution immediately
preceding the `Node failed to start!` failure in `TaskTest`) rather than taking
the PR description at face value, traced the `AbstractSeaTunnelServerTest`
subclass hierarchy to confirm both cited flaky test classes benefit from the
base-class fix without needing individual changes, and confirmed the one
plausible near-miss (`SourceSplitEnumeratorTaskTest`'s
`createUnresolvedAddress` calls) is correctly out of scope since that path
never performs DNS resolution. The only residual gap is the REST-client
`localhost` URLs noted above, which is a reasonable scope boundary given the
available evidence.
--
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]
