SEZ9 commented on PR #12298: URL: https://github.com/apache/seatunnel/pull/12298#issuecomment-5807064024
Thanks for trimming this down — the branch now carries just the fix itself (`HttpConfig.java`, `JettyService.java`, `RestApiIT.java`), with the unrelated work split out. That resolves the scope concern. The fix content in `d1cf006305aa` is unchanged from the previous round, so the earlier items still stand. One is blocking; the rest are minor and can go in the same push. **Blocking** - **Shared mutable config (`JettyService.java`)** — writing the bound port back into the `HttpConfig` held by `SeaTunnelConfig` means every member built from the same config object ends up advertising the last-bound port. Please keep the actually-bound port in per-node state (e.g. owned by the `JettyService`/node instance) and have `GetNodeHttpPortOperation` read from there, or ensure each member gets its own `HttpConfig` copy before the write-back. A test covering two members sharing one `SeaTunnelConfig` would be good either way. **Minor** - **Write-back when HTTP is disabled (`JettyService.java`)** — dynamic-port selection and write-back currently run even for HTTPS-only deployments, so peers get a port nothing listens on. Please guard it on the HTTP connector being enabled. - **Docs (`HttpConfig.java`)** — `getPort()` now returns the bound port instead of the configured one, but that is only noted in the field Javadoc and the PR description. Please reflect it in the REST docs so users know what `GET /logs` / `GET /loggers?scope=cluster` will report. - **PR description** — the local-mode REST log line in `ClientExecuteCommand` still prints the configured port, so please narrow the description to what the fix covers (or extend the fix, your call). - **Tests (`RestApiIT.java`)** — `testDynamicHttpPortIsResolvableByPeers` implicitly depends on both members sharing one log directory and on node2 inheriting port 8080 from the test `seatunnel.yaml`. Please make both explicit so the test doesn't silently break if the layout changes. - **Comment nit (`JettyService.java`)** — hoisting `HttpConfig` into a local is good; have the inline comment point at `LogService`/`LoggerLevelService` as the consumers of the written-back port. Once the blocking item lands I'm happy to take another look. <!-- streview-comment:1285 --> -- 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]
