DanielLeens commented on PR #10940:
URL: https://github.com/apache/seatunnel/pull/10940#issuecomment-4528176353
Thanks for working on this. I pulled the latest head locally and traced the
actual engine E2E startup path.
# What This PR Fixes
- User pain: a set of engine E2E tests starts SeaTunnel cluster nodes even
though those test cases do not exercise the REST layer. Leaving HTTP enabled
there can introduce unnecessary startup noise and flaky port-related failures
in CI.
- Fix approach: the PR explicitly disables `engine.http` in the remaining
engine E2E test classes that only need the cluster/job runtime path.
- One-line summary: this is a targeted test-stability cleanup that removes
unnecessary Jetty startup from non-REST engine E2Es.
Simple example:
- Before this change, `ClusterIT` and the fault-tolerance E2Es would start
Hazelcast nodes together with the HTTP server even though the assertions only
use the engine client / job path.
- After this change, those tests still boot the cluster and run the same job
logic, but they no longer pay the extra REST-server startup cost.
# Runtime Chain Rechecked
```text
Engine E2E bootstrap
-> ConfigProvider.locateAndGetSeaTunnelConfig()
-> SeaTunnelServerStarter.createHazelcastInstance(seaTunnelConfig)
[seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/SeaTunnelServerStarter.java:43-72]
-> initializeHazelcastInstance(...)
-> SeaTunnelServer is created through SeaTunnelNodeContext
HTTP server startup condition
-> SeaTunnelServer.start()
[seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/SeaTunnelServer.java:176-178]
-> start Jetty only when http.enabled or https.enabled is true
Test path in this PR
-> test class sets
seaTunnelConfig.getEngineConfig().getHttpConfig().setEnabled(false)
-> cluster/job execution still uses SeaTunnelClient, Hazelcast membership,
and job operations
-> REST-specific coverage remains in dedicated tests such as RestApiIT /
PendingJobsRestIT where HTTP is explicitly enabled
```
# Key Findings
- The normal path really does hit this change, because these tests
instantiate engine nodes through `SeaTunnelServerStarter`.
- The modified tests do not rely on HTTP endpoints for their assertions. For
example, `SeaTunnelClient.getClusterHealthMetrics()` uses the client codec
path, not Jetty-backed REST.
- This is consistent with the existing test suite: many other non-REST
engine E2Es already disable HTTP explicitly for the same reason.
- The latest `Build` check is green, which is what I wanted to see for a
flaky-test cleanup PR.
# Findings
No blocking issue found in the current head.
# Merge Decision
### Conclusion: can merge
1. Blocking items
- None.
2. Suggested follow-up
- None from my side.
Overall, this looks like a good cleanup. It reduces unnecessary moving parts
in the affected E2Es while preserving dedicated REST coverage elsewhere.
--
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]
