SEZ9 commented on PR #12298: URL: https://github.com/apache/seatunnel/pull/12298#issuecomment-5707691329
Thanks for the follow-up. I agree the two red e2e jobs (`NebulaGraphIT`, `OpengaussCDCIT`) are not reachable from a change confined to the `JettyService` constructor and the `HttpConfig.port` field, and tracking them separately rather than gating this PR on them is the right call. That said, the earlier review points on this PR are still open from my side, so before this is merge-ready I'd like to see them either addressed or explicitly answered: 1. **Shared mutable config (MEDIUM)** — writing the bound port back into `HttpConfig` means every member built from the same `SeaTunnelConfig` advertises the last-bound port. Please either store the bound port on the `JettyService` instance (or another per-member holder) and have the callers read it from there, or explain why one config instance per member is guaranteed in all deployment paths. 2. **HTTP disabled / HTTPS-only** — dynamic-port selection and the write-back should be skipped when the HTTP connector is not enabled, otherwise we advertise a port nothing is listening on. 3. **Docs** — the change in meaning of `HttpConfig.getPort()` (bound port vs. configured port) is currently only in the field Javadoc and the PR description; please reflect it in the REST docs as well. 4. **PR description** — the local-mode REST log line in `ClientExecuteCommand` still prints the configured port, so please narrow the description to what the fix actually covers (or extend the fix to that path). 5. **Tests in `RestApiIT`** — please make the two implicit assumptions explicit: that both members share one log directory for the new assertion, and that node2 gets port 8080 from the test `seatunnel.yaml`. A short comment or an explicit setup step is enough so a future layout change fails loudly instead of silently. 6. **Style (optional)** — the inline comment next to the hoisted `HttpConfig` local could name `LogService`/`LoggerLevelService` as the consumers so the next reader doesn't have to grep. Item 1 is the one I'd consider blocking; the rest are small. Happy to take another look once those are pushed. <!-- streview-comment:1112 --> -- 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]
