SEZ9 commented on PR #12298: URL: https://github.com/apache/seatunnel/pull/12298#issuecomment-5825943894
Quick status on `d1cf006305aa` from my side: **Blocking (F1):** the bound port is still written back into the shared `HttpConfig` instance, so any members built from the same `SeaTunnelConfig` all advertise whichever port was bound last. Two acceptable ways out, your call: - keep the runtime bound port as per-node state in `JettyService` and have the callers read it from there, or - keep the write-back, but document the "one `HttpConfig` per member" assumption explicitly and add a test that pins it. Please let me know which direction you're taking so the next pass can be a full one. **Low, can land in the same push:** - F4: skip the dynamic-port selection and write-back when the HTTP connector is disabled (HTTPS-only), so we don't advertise a port nothing listens on. - F2 / F3: reflect the new `HttpConfig.getPort()` semantics (bound port rather than configured port) in the REST docs, not only in the field Javadoc and the PR description. - F5: either update the local-mode REST log line in `ClientExecuteCommand` too, or narrow the PR description so it doesn't claim more than the change covers. - F6 / F8: make the `RestApiIT` assumptions explicit — both members sharing one log directory, and node2 inheriting port 8080 from the test `seatunnel.yaml` — so the test fails loudly rather than silently if the layout changes. - F7: minor — have the inline comment in the `JettyService` constructor point at `LogService` / `LoggerLevelService` so the callers are easy to find. **CI:** the automated review noted that the apache-side `Build` check on this head shows `FAILURE`. Could you confirm whether the fork's actual run for `d1cf006305aa` is green, and whether this is the pre-existing `all-connectors-it-2` (#12344) situation or something new? If it's the former it doesn't affect merge readiness; if it's new I'd like to know before re-reviewing. Thanks — ping me once F1 is addressed and I'll take a fresh look at the whole set. <!-- streview-comment:1302 --> -- 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]
