SEZ9 commented on PR #12298: URL: https://github.com/apache/seatunnel/pull/12298#issuecomment-5724189976
Thanks for the update on this thread. Quick recap of where I land after the latest review pass, plus what I still need before this can go in. **Shared mutable `HttpConfig` (blocking).** The concern is now confirmed against in-tree code rather than a hypothetical: `ServerExecuteCommandTest.testMemberList` builds a single `SeaTunnelConfig`, flips `setEnableDynamicPort(true)` on it, and hands that same instance to five `createMasterHazelcastInstance`/`createWorkerHazelcastInstance` calls. Each `JettyService` constructor then reads `httpConfig.getPort()` (whatever the previous member just wrote), probes forward, and overwrites the same field. So `HttpConfig.getPort()` cannot stand in for "this node's bound port" whenever the config object is shared. Please move the bound port into per-node runtime state on `JettyService` (e.g. a `getBoundPort()` accessor) and have the consumers — `LogService`/`LoggerLevelService` — read it from there instead of from `HttpConfig`. With that in place the write-back into `HttpConfig.port` should go away entirely, which also resolves the `getPort()` semantic drift and the docs asks that cam e with it. **Remaining smaller items, still open:** - Skip the dynamic-port probe/advertise path when the HTTP connector is disabled (HTTPS-only); today it advertises a port nothing listens on. - The local-mode REST log line in `ClientExecuteCommand` still prints the configured port — either cover it or narrow the PR description accordingly. - In `RestApiIT`, make the two implicit assumptions explicit: that both members share one log directory, and that node2 inherits port 8080 from the test `seatunnel.yaml`. A comment plus an explicit assertion/setup is enough. - If any part of the `getPort()` semantic change survives the refactor, please reflect it in the REST docs rather than only the field Javadoc / PR description. On CI, the picture as described is fine by me: `all-connectors-it-1` cleared on rerun, the remaining `all-connectors-it-2` red (`OpengaussCDCIT.testAddFieldWithRestore`, tracked as #12344) is unrelated to this diff, and `RestApiIT` showing 23 run / 0 failures / 0 skipped on both JDKs confirms `testDynamicHttpPortIsResolvableByPeers` actually executed. Once the per-node bound-port refactor is pushed I'll take another look promptly. <!-- streview-comment:1128 --> -- 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]
