SEZ9 commented on PR #12298: URL: https://github.com/apache/seatunnel/pull/12298#issuecomment-5852020736
Thanks for the update. Moving the bound port into per-member runtime state read from Jetty's `ServerConnector`, rather than writing it back into the shared `HttpConfig` bean, is the direction I was hoping for on F1. I haven't yet been able to confirm the current state of the earlier points against `6f7a6dd94601`, so a few questions to help close them out: 1. **F1 – shared mutable config.** Could you point me at the `JettyService` hunk where the write-back into `HttpConfig` was removed, so I can verify multiple members built from one `SeaTunnelConfig` no longer advertise the last-bound port? 2. **F2 / F3 – `HttpConfig.getPort()` semantics.** If `HttpConfig` is no longer mutated at runtime, `getPort()` should mean "configured port" again. Please confirm, and make sure the field Javadoc, the REST docs and the PR description no longer describe the old write-back behaviour. 3. **F4 – HTTP connector disabled (HTTPS-only).** Please confirm that a port is only advertised from a connector that actually started, and nothing is advertised when the HTTP connector is disabled. 4. **F5 – PR description scope.** Does this change also cover the local-mode REST log line in `ClientExecuteCommand`? If not, please narrow the PR description accordingly. 5. **F6 / F8 – `RestApiIT` assumptions.** Are the shared log directory and node2's port 8080 now made explicit in `testDynamicHttpPortIsResolvableByPeers` (a short comment plus an explicit assertion/config value)? If not, please add them so the test doesn't silently break if the layout changes. 6. **F7 – nit.** If the `HttpConfig` local in the `JettyService` constructor is still there, a brief comment pointing at its callers would help the next reader; feel free to skip if that code has moved. Happy to take another pass once I can see the relevant hunks. <!-- streview-comment:1341 --> -- 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]
