SEZ9 commented on PR #12298:
URL: https://github.com/apache/seatunnel/pull/12298#issuecomment-5788057115
Agreed, and thanks for the correction — my recap was incomplete. Since the
head is still `d41bfb7122` and the diff on the fix's own files is unchanged,
there is nothing new to re-review. Let's treat this as the single checklist,
folding the two items I missed into my F-items.
For the author, in order:
1. **Scope (blocker)** – trim the branch to the two REST-port commits
(`2101b81719`, `d671c5d867`) so the unrelated commits drop out of the diff.
2. **F1 (Medium, Robustness)** – `JettyService` writes the bound port back
into the shared `HttpConfig`, so members built from one config object all
advertise the last-bound port. Please keep the runtime bound port in per-member
state, or explain why a shared instance cannot occur in practice.
3. **F2/F3 + doc limitation (Medium, Docs)** – in one doc edit, reflect the
new `HttpConfig.getPort()` semantics in the REST docs and remove the
`rest-api-v2.md` sentence (both `en` and `zh`) saying a cluster-scope
`/loggers` request does not reach members that took a different port through
`enable-dynamic-port`. That is exactly the bug this PR fixes, so it becomes
false once this lands.
4. **Low items**
- **F4 (Logic)** – gate dynamic-port selection and the write-back on the
HTTP connector actually being enabled; HTTPS-only currently advertises a port
nothing listens on.
- **F5 (Docs)** – the local-mode REST log line in `ClientExecuteCommand`
still prints the configured port; fix it or narrow the PR description.
- **F6/F8 + test coverage (Test)** – make the shared-log-dir and node2
port-8080 assumptions in `testDynamicHttpPortIsResolvableByPeers` explicit;
tighten `testLoggers` so it would catch a "node1 answered twice" regression on
the cluster-scope path; and update the stale "configured on the node" wording
in `GetNodeHttpPortOperationTest#testReturnsConfiguredHttpPort` and the
operation's Javadoc. To be explicit: this item is accepted only in part — I'd
drop the separate unit-level test of the write-back, since
`testDynamicHttpPortIsResolvableByPeers` already covers the
fallback-and-resolve path end to end.
- **F7 (Style)** – the inline comment where `HttpConfig` is hoisted into
a local could name `LogService`/`LoggerLevelService` as the callers.
Once a new head with these changes is pushed I'll review the delta. If you'd
prefer to split F1 into a follow-up PR, say so and we can re-scope together.
<!-- streview-comment:1250 -->
--
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]
