DanielLeens commented on PR #11545: URL: https://github.com/apache/seatunnel/pull/11545#issuecomment-5846256933
Thanks for working through this carefully, @SEZ9 — pointers below for each item, then answers to your three asks. Evidence, all pulled from the raw file content at `50223b3014626` rather than re-describing the diff: - **F5** (fail-fast Java-version check): `seatunnel.sh:112-118` (and the matching block in `seatunnel-cluster.sh`). It parses `java -version`, exits 1 with the actionable message below Java 11, and runs before the module-flag loop below it, so it always executes first. - **F1** (launcher-side idempotent flag append): `seatunnel.sh:120-135`. The six flags are appended in a loop that checks each one as a space-delimited substring of the already-assembled `JAVA_OPTS` before adding it, so a preserved `config/jvm_*_options` from an old install only gets the flags it's missing, never a duplicate. - **F2** (silent Kerberos reload failure): `KuduUtil.java:151-163` and `PaimonSecurityContext.java:139-159`. Both now log at ERROR with the exact `--add-exports` flag when the reflective refresh throws `IllegalAccessException`. One asymmetry survives, unrelated to what you asked about this round: in `KuduUtil.java`, `KerberosName.resetDefaultRealm()` (line 143) sits outside any try/catch, while `PaimonSecurityContext.java` (lines 141-143) wraps both the refresh and the realm-reset call in one try block with its own `ReflectiveOperationException` handler. That's Issue 2 in my table — still open, not newly resolved, just flagging so it isn't conflated with F2. - **F4/F7** (old 2.3.13 on JDK 17): `upgrade_compatibility.yml:56-60` — `java-version: "11"` with the inline comment explaining why the old release needs its own bundled JDK 11. - **F1's incompatible-changes.md note**: good addition, but I don't have it in a commit yet — that file is untouched since my last review. I'll fold the note in with the Issue 1 fix below rather than land a doc-only commit first. On what changed between `2c87f42f2b236` and `50223b3014626` — exactly one commit, touching only `backend.yml`'s two unit-test steps (+7/-2), which repeats the six flags directly in `SUREFIRE_JVM_ARGS` so the CLI-level override stops defeating the `pom.xml` profile's value for that one job. That's the "half": it fixed the JDK 17 unit-test lane specifically. The other half — the profile itself (`pom.xml:1224-1232`, the `jdk9-plus-test-opens` profile's `<jdk>[9,)</jdk>` activation) silently deactivating the default `release` profile and dropping `seatunnel-dist` out of the reactor — is untouched, and it's reproducing right now on the `dependency-license` job (`Could not find the selected project in the reactor: seatunnel-dist`). On your three asks: 1. Agreed — and this is exactly Issue 1's proposed fix from my last review, independently: replace the `<jdk>[9,)</jdk>` activation with an always-on property (Java 11 is the hard floor now, so there's no JDK-version case left to gate on) and reference it from Surefire/Failsafe `<argLine>` instead of profile activation. That removes the default-profile-deactivation side effect entirely, and once it's in, the `backend.yml` duplication from this round's commit becomes redundant and I'll revert it. 2. Once (1) lands I'll post a green fork run on plain JDK 11 and JDK 17 with no `JAVA_TOOL_OPTIONS` at the workflow level. A run today wouldn't prove what you're asking it to — `backend.yml`'s unit-test job still needs the explicit `SUREFIRE_JVM_ARGS` workaround because of the profile bug. 3. On F3: I'd rather document than narrow further. `config/jvm_*_options` apply process-wide to Master/Worker/Client JVMs that dynamically load whatever connector jars a user installs via `bin/install-plugin.sh`, so the launcher has no way to know at packaging time which of those will need reflective access — scoping the opens to "only the connectors shipped today" would just break the next connector that needs one. I'll add the per-line comments you asked for back on 09-19 naming the known consumers (Hazelcast serialization for the `java.lang`/`java.net`/`java.nio`/`java.util`/`sun.nio.ch` opens, the plugin `URLClassLoader#addURL` trick, Kerberos krb5 reload for the jgss export) so the blanket opens are auditable, rather than trying to pare the list down. I'll push the profile fix, the reverted `backend.yml` duplication, and the `incompatible-changes.md` note together, then ping you with the new head and the plain-JDK CI run. -- 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]
