SEZ9 commented on PR #11545:
URL: https://github.com/apache/seatunnel/pull/11545#issuecomment-5738610203
Re-review at `00794fa12ee`. Compared against `81f12fffbd8`, the PR's own
content is unchanged apart from shifted context lines in
`docs/{en,zh}/introduction/concepts/incompatible-changes.md` from the `Merge
branch 'dev' into dev-ci-jdk17-20260723` merge, so this round is a fresh look
at the same diff.
- **PR11545-F1 / PR11545-F5** – With the `--add-opens`/`--add-exports` flags
also injected idempotently by the launcher scripts (so a preserved config
directory across an in-place upgrade cannot drop them) and the fail-fast
Java-version pre-check in place, I consider both of these addressed. Thanks.
- **PR11545-F2 (silent Kerberos reflective-reload failure)** – I still don't
see anything that detects or logs when the reflective
`sun.security.krb5.Config` reload is blocked because the module flags are
missing. A warning (or a hard failure when Kerberos is actually configured)
would make the `config/jvm_client_options` comment true in practice rather than
only documented.
- **PR11545-F3 (blanket opens to `ALL-UNNAMED`)** – I understand this is the
pragmatic choice, but please add a short comment in `config/jvm_master_options`
(and the other option files) naming which component needs each
`--add-opens`/`--add-exports` line, so they can be pruned later instead of
accreting.
- **PR11545-F4 / PR11545-F7 (old 2.3.13 release on JDK 17 in
`upgrade_compatibility.yml`)** – Same concern from two angles. Either run the
old release on JDK 11 (the realistic "upgrade from an older runtime" scenario),
or, if it must run on 17, explain in the workflow why only the jgss export is
sufficient for that build to start. Right now I can't tell whether a green run
proves compatibility or just that the old cluster happened not to touch the
blocked internals.
- **PR11545-F6 / PR11545-F8 (workflow-level `JAVA_TOOL_OPTIONS` in
`backend.yml`)** – A global env var means CI no longer exercises a plain JDK
11/17 and every child JVM emits `Picked up JAVA_TOOL_OPTIONS` noise. Please
scope it to the specific jobs/steps that need it, so local developer builds and
CI see the same thing.
Once F2 has a detection path and the `upgrade_compatibility.yml` /
`backend.yml` scoping questions are settled, I'm happy to do a final pass.
Thanks for the careful work on this.
<!-- streview-comment:1151 -->
--
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]
