SEZ9 commented on PR #12298:
URL: https://github.com/apache/seatunnel/pull/12298#issuecomment-5770994025
Thanks for the new push. Summarising where head
`d41bfb712266506ce1263bd87acbbfab4a0d144c` stands so the remaining work is in
one place.
CI is green on this head (run `35529066998`), but the diff
`da60137cc5..d41bfb7122` on `JettyService.java`, `HttpConfig.java`,
`GetNodeHttpPortOperation.java`, `RestApiIT.java` and
`docs/{en,zh}/engines/zeta/rest-api-v2.md` is empty, so the items from the
previous round are still open.
**Scope (main blocker)**
- The PR grew from 15 files / ~38 KB to 33 files / ~122 KB (`+1531/-91`)
with four new commits that are unrelated to the REST-port fix, while the PR's
own three files (`HttpConfig.java`, `JettyService.java`, `RestApiIT.java`) are
`+62/-10`. Please drop the unrelated commits (e.g. rebase onto the target
branch) so the PR contains only the REST-port change. Green CI does not change
this.
**Robustness (Medium)**
- **F1** – `JettyService` writing the bound port back into `HttpConfig`
means every member built from the same `SeaTunnelConfig` reports the last-bound
port. Please either keep the bound port in per-node runtime state (and have
`GetNodeHttpPortOperation` read it from there), or explain why a shared
`SeaTunnelConfig` cannot occur in practice. `LoggerLevelService#nodeId()` also
reads `HttpConfig.getPort()` directly, so whichever approach you pick should
cover that caller too.
**Low items**
- **F4** – dynamic-port selection and write-back run even when only HTTPS is
enabled, so the node advertises a port nothing listens on. Please guard the
write-back on the HTTP connector actually being enabled.
- **F2/F3** – `HttpConfig.getPort()` now returns the bound port rather than
the configured one; this is only recorded in the field Javadoc and the PR
description. Please add a short note to
`docs/{en,zh}/engines/zeta/rest-api-v2.md`.
- **F5** – the local-mode REST log line in `ClientExecuteCommand` still
prints the configured port. Please either fix that line or narrow the PR
description so it doesn't claim local mode is covered.
- **F6** – `testDynamicHttpPortIsResolvableByPeers` relies on both members
sharing one log directory. Please make that explicit (a comment plus an
assertion on the log location).
- **F8** – the test also implicitly relies on node2 inheriting port 8080
from the test `seatunnel.yaml`. Please set the port explicitly in the test
setup so the collision is intentional and visible.
- **F7 (nit)** – the inline comment in the `JettyService` constructor could
point at `LogService`/`LoggerLevelService` as the callers that depend on the
written-back port.
Once the scope is trimmed and F1 is addressed, I'm happy to do a final pass
on the rest. If you disagree with the F1 approach, let's discuss here before
you rework it.
<!-- streview-comment:1230 -->
--
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]
