DanielLeens commented on PR #12298: URL: https://github.com/apache/seatunnel/pull/12298#issuecomment-5713046832
Thanks for the consolidated list, and for the further CI digging — the attempt-carry-forward retraction on `NebulaGraphIT` is a good catch, I'd been repeating the same inflated per-leg counts in my own comment. On CI: agreed on the current picture. `all-connectors-it-1` cleared on rerun, `all-connectors-it-2` (`OpengaussCDCIT.testAddFieldWithRestore`, tracked as #12344) is the one remaining red job and it's unreachable from a diff confined to `JettyService`'s constructor and the `HttpConfig.port` field. `RestApiIT`: 23 run, 0 failures, 0 skipped on both JDKs confirms `testDynamicHttpPortIsResolvableByPeers` genuinely passed rather than being silently skipped. On the six open items — I hadn't directly answered your review round yet, so let me do that now rather than let it sit. **Item 1 (shared mutable `HttpConfig`) — confirmed real, and I agree it's blocking.** I checked this against source rather than taking it on the description: `ServerExecuteCommandTest.testMemberList` (`seatunnel-core/seatunnel-starter/src/test/java/org/apache/seatunnel/core/starter/seatunnel/command/ServerExecuteCommandTest.java:53-66`) builds exactly one `SeaTunnelConfig` via `ConfigProvider.locateAndGetSeaTunnelConfig()`, calls `seaTunnelConfig.getEngineConfig().getHttpConfig().setEnableDynamicPort(true)` on it, and then passes that same instance into five `createMasterHazelcastInstance`/`createWorkerHazelcastInstance` calls (2 master, 3 worker). The classpath default `seatunnel.yaml` (`seatunnel-engine-common/src/main/resources/seatunnel.yaml:47-48`) ships `enable-http: true`, `port: 8080`, so all five `SeaTunnelServer.init()` calls construct a `JettyService` (`SeaTunnelServer.java:188-190`) against the identical `HttpConfig` object. Each constructor reads `httpConfig.getPort ()` (`JettyService.java:112`) — which is whatever the previous instance in the loop just wrote — probes forward from there, and overwrites the same field (`JettyService.java:118`). So the "every node owns its own `SeaTunnelConfig`" assumption my earlier review rounds relied on is not universal, and this is a real, already-in-tree counterexample rather than a hypothetical. That undercuts using `HttpConfig.getPort()` as a stand-in for "this node's bound port" whenever a config instance is shared, which is exactly the pattern this in-tree test exercises. I'm revising my earlier conclusion here: your suggested direction — hold the bound port as per-node runtime state (e.g. `JettyService#getBoundPort()`, read by `GetNodeHttpPortOperation`, falling back to `HttpConfig.getPort()` when Jetty isn't running) rather than mutating the parsed config bean — is the right fix, and I'd treat this as blocking too. Items 2-6 I have no disagreement with: guarding the dynamic-port write-back behind `httpConfig.isEnabled()` so an HTTPS-only node doesn't advertise an HTTP port nothing listens on, updating the REST docs for the `getPort()` meaning change, narrowing the PR description's `ClientExecuteCommand` claim, making `RestApiIT`'s shared-log-directory and inherited-port-8080 assumptions explicit in the test itself, and naming `LogService`/`LoggerLevelService` in the write-back comment — all sound, all small. So: once Item 1 is addressed (moving to per-node runtime state instead of the shared config bean) along with items 2-6, I think this is in good shape. Happy to take another pass once that's pushed. -- 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]
