SEZ9 commented on PR #12298:
URL: https://github.com/apache/seatunnel/pull/12298#issuecomment-5674519392
Thanks @DanielLeens for the second full pass, and in particular for diffing
the old SHA against the rebased head rather than carrying the previous round's
verdict forward — that is exactly the check that catches a force-push that
quietly changed logic.
**Issue 2 (`HttpConfig.port` not `volatile`) — taken**, in `66cf865bf`. I
went with `volatile` rather than only a comment, since the field genuinely is
written by the init thread and read from Hazelcast operation-service threads,
and a plain `int` makes the safety argument depend on incidental startup
synchronization that a later refactor is free to remove:
```java
/**
* The REST port. Unlike every other field here this one is written after
the config has been
* parsed - {@code JettyService} writes the port it actually bound back into
it when dynamic
* ports are enabled - and it is read from the Hazelcast operation threads
that answer {@code
* GetNodeHttpPortOperation}, so it is declared volatile to make that write
visible to them.
*/
private volatile int port =
ServerConfigOptions.MasterServerConfigOptions.PORT.defaultValue();
```
The javadoc states *why* it is different from its neighbours, which is the
part that protects it from being "cleaned up" later. The cost is a single
volatile read per `GetNodeHttpPortOperation`, i.e. per cluster fan-out — not on
any data path.
**Issue 1 (`incompatible-changes.md` entry) — I would rather not add one,
and here is my reasoning.**
`HttpConfig.getPort()` is an internal engine accessor, not user-facing
configuration surface: it is not a config option name, not a REST response
field, and not part of any published API. Users do not read it; three in-tree
call sites do (`LogService`, `LoggerLevelService`, and the local-mode log line
in `ClientExecuteCommand`), and as your own 1.2 analysis established, all three
*want* the bound port and none depended on the configured value.
So the only change a user can observe is that `GET /logs` and `GET
/loggers?scope=cluster` now reach a dynamically-bound node instead of silently
skipping it. There is no configuration to change, no behaviour to opt out of,
and no migration to perform — the old behaviour is not something anyone could
have been relying on deliberately. An entry in `incompatible-changes.md` would
tell an upgrading user "something you depend on may break, here is what to do",
when in fact the answer is "a broken endpoint now works". I think that dilutes
a document whose value comes from every entry being actionable.
I do agree the distinction is worth recording somewhere, which is why the PR
description carries the before/after cluster example — happy to move that into
the release notes instead if a committer prefers, but I do not think it belongs
in the incompatible-changes list. If a maintainer disagrees, I will add it
without further argument; I would just rather the disagreement be explicit than
silently resolved by adding an entry I do not think is accurate.
Contrast with #12299 in the same area, where I *did* add
`incompatible-changes.md` entries in both languages: there, a new option
(`log-response-max-size-mb`) changes a default from unlimited to bounded, so an
existing user really does see less data than before and really does have an
action to take (`log-response-max-size-mb: 0`). That is what the doc is for.
CI on the fork is re-running for the new head; I will report back when it
settles.
--
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]
