DanielLeens commented on PR #11545:
URL: https://github.com/apache/seatunnel/pull/11545#issuecomment-5995579828
Thanks @SEZ9. I pushed the items you listed as the preconditions for a
re-review. Head is now `4d07dbeae` (0 commits behind `dev`, no conflict). Each
point below is checkable against that head.
**What changed**
- Merge of `apache/dev`: the only conflicts were in
`docs/{en,zh}/introduction/concepts/incompatible-changes.md`, where dev and
this PR both added an entry at the top of the `dev` section. Every hunk keeps
both sides unchanged; no source file was hand-resolved.
- `4ce5edfba` (4 files: `backend.yml`, `upgrade_compatibility.yml`, and the
en/zh `incompatible-changes.md`):
- **F6 comment fix**: the `backend.yml` header and unit-test step comments
no longer name the deleted `jdk9-plus-test-opens` profile; they point at the
`surefire.module.args` property.
- **F6 behaviour change you should look at**: the unit-test lane's
`SUREFIRE_JVM_ARGS` no longer repeats the six module flags (it keeps only
`-Xmx`, metaspace and encoding). They now come only from the pom property,
which the `-Dsurefire.jvm.args` override cannot replace. I did this on purpose
so CI exercises the same path as a local `./mvnw test`; the flip side is that
if the property were wrong, the JDK 17 unit lanes would go red instead of being
masked. That is untested until the run below finishes.
- **F1 docs**: both locales now say the launcher scripts append the
mandatory `--add-opens`/`--add-exports` flags themselves and skip ones already
present, so a preserved `config/` directory still works, and that a JVM older
than 11 gets an explicit `SeaTunnel requires Java 11 or newer` message.
**CI evidence**: fork run
https://github.com/DanielLeens/seatunnel/actions/runs/37317968844 on
`4d07dbeae`. It is still in progress, so I am not claiming any result yet.
Earlier runs on this PR were cancelled, so none of them was usable.
**F4 / F7 (old 2.3.13 release)**: it is not run on JDK 17.
`upgrade_compatibility.yml` is pinned to `java-version: "11"` (commit
`be5492399`, with the reason in a comment), and it passes no `--add-opens`
flags. My reasoning for why that starts cleanly is from documented JDK
behaviour, not from a green run on this head: illegal reflective access is
permitted by default through JDK 15 (JEP 396 made it denied from 16, JEP 403
removed the option in 17), so a Java 8-era distribution on 11 should not need
the flags. The `JAVA_TOOL_OPTIONS` jgss export on that job is left over and not
needed for startup; I added a comment saying so rather than removing it in this
round. If you would rather it be deleted, I will do that.
**F3 (evidence for the flag list)**: I only have a concrete observed failure
for one flag.
- `--add-opens=java.base/java.net=ALL-UNNAMED`: `ReflectionUtilsTest` on
`unit-test (17)` of fork run 35357435427 failed with
`InaccessibleObjectException: Unable to make protected void
java.net.URLClassLoader.addURL(java.net.URL) accessible: module java.base does
not "opens java.net" to unnamed module`. That is the plugin classloader path
(`AbstractPluginDiscovery`), not Hazelcast.
- For `java.lang`, `java.nio`, `java.util`, `sun.nio.ch` and the
`sun.security.krb5` export, I do not have a captured failing stack trace. They
were kept because Hazelcast (the shaded 5.1 line) reflects into JDK internals,
but I did not prove each one necessary, and I have not re-checked Hazelcast's
own JDK 16+ requirements against the shaded version in this pass.
- Because the unit lane now reads the flags from a single pom property,
narrowing is cheap to test empirically: drop one flag per run and see which JDK
17 lanes fail. I have not done that. If you want it, I will run that bisect on
the fork and post the list that survives, instead of arguing from docs.
--
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]
