SEZ9 commented on PR #12298: URL: https://github.com/apache/seatunnel/pull/12298#issuecomment-5754543883
Thanks for the update. I re-checked the head and the four files that carry the fix (`JettyService.java`, `HttpConfig.java`, `GetNodeHttpPortOperation.java`, `RestApiIT.java`) are unchanged between `d671c5d` and `da60137`, so the points from the previous round are still open. To be clear on my side: the earlier "ready to merge" wording was premature — the shared mutable `HttpConfig` concern is real and I should have flagged it in the review body itself. Remaining asks, in priority order: 1. **Shared mutable config (PR12298-F1, MEDIUM)** — writing the bound port back into `HttpConfig` means every member built from the same `SeaTunnelConfig` advertises whichever port was bound last. Please move the bound-port state to per-node runtime state (e.g. owned by `JettyService` / the node) and have `GetNodeHttpPortOperation` read from there instead of from the config object. If you believe one `SeaTunnelConfig` per member is guaranteed in all deployment paths, please spell out why in the PR so we can evaluate it. 2. **HTTPS-only case (PR12298-F4)** — dynamic-port selection and the write-back should only run when the HTTP connector is actually enabled; otherwise we advertise a port nothing listens on. 3. **Docs (PR12298-F2 / F3)** — the change in what `getPort()` returns (bound vs configured) is currently only in the field Javadoc and the PR description. Please update the REST docs so the behaviour with `enable-dynamic-port: true` is documented where users look for it. 4. **PR description (PR12298-F5)** — the local-mode REST log line in `ClientExecuteCommand` still prints the configured port, so please narrow the description to what the change actually covers (or include that line in the fix). 5. **e2e test (PR12298-F6 / F8)** — `testDynamicHttpPortIsResolvableByPeers` implicitly relies on both members sharing one log directory and on node2 inheriting 8080 from the test `seatunnel.yaml`. Please make both expectations explicit (assert/set them in the test) so a layout change doesn't silently break the assertion. 6. **Comment (PR12298-F7, nit)** — the inline comment next to the hoisted `HttpConfig` local could point at `LogService` / `LoggerLevelService` as the consumers. Once 1 and 2 are addressed I'm happy to take another full pass quickly; 3–6 are small and can land in the same push. <!-- streview-comment:1199 --> -- 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]
