DanielLeens commented on PR #11669:
URL: https://github.com/apache/seatunnel/pull/11669#issuecomment-5216078931
Sorry for missing this in my last review — that's a more fundamental problem
than what I flagged, and I want to correct the record rather than let my Issue
1 stand as the primary blocker.
I pulled `YamlSeaTunnelDomConfigProcessor.parseHttpConfig()` at the current
head (`f2880e893`) to check @SEZ9's claim directly. It's confirmed: the
`if`/`else if` chain in `parseHttpConfig()` handles `port`, `context-path`,
`enable-http`, `enable-dynamic-port`, `port-range`, `enable-https`,
`https-port`, `key-store-path`, etc., but there is no branch for
`public-url`/`ServerConfigOptions.MasterServerConfigOptions.PUBLIC_URL`
anywhere in that method. That means a user setting `public-url` in
`hazelcast.yaml` today has it silently dropped — `httpConfig.getPublicUrl()` is
always `null` on the code path this PR relies on, regardless of what the user
configures.
That changes the severity picture from my last review. My Issue 1 ("the fix
only really works for a single HTTP-serving node, and breaks multi-node
discovery") implicitly assumed `public-url` *can* be set and takes effect for
at least the single-node case. It can't — the option can never reach
`LogService` at all as written, so the feature doesn't work for *any* topology
yet, not just multi-node ones. @SEZ9's framing ("as written the feature is dead
code") is the more accurate one and should be the lead blocker, with my
original multi-node concern folded in underneath it as the next thing to get
right once wiring is fixed.
Updated blocker list for this head:
1. (New primary blocker, was missed by me) Wire `public-url` into
`YamlSeaTunnelDomConfigProcessor.parseHttpConfig()` so
`HttpConfig.getPublicUrl()` actually reflects the configured value.
2. (My original Issue 1, still applies once #1 is fixed) Separate the
internal per-member discovery URL from the externally-facing `public-url`-based
link, so discovery still targets each member's real `host:port` while only the
returned `logLink` uses `public-url` — otherwise every member's link collapses
to the same URL and `contextPath` gets dropped, per @SEZ9's trace.
3. My original Issues 2-4 (spotless formatting, missing unit test for the
`public-url` branch, docs) still stand once 1 and 2 are addressed.
This is still a solid first PR with the right diagnosis of the underlying
problem (#11662) — the wiring gap is a small, mechanical fix, not a design
issue. Looking forward to the update, and thanks again @SEZ9 for catching what
I missed.
--
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]
