DanielLeens commented on PR #11545: URL: https://github.com/apache/seatunnel/pull/11545#issuecomment-5633094671
Thanks for the structured ask, @SEZ9 — a per-ID checklist is fairer than making you reconstruct it from prose across several comments. I re-verified all five "fixed" claims directly against the current head (`fbd46ce70df3`, which per my last review is only a `dev`-merge on top of `c32b89fd4b1d` with no changes to this PR's own files) rather than just repeating the Aug 28 comment, since a merge commit is exactly the kind of thing that could silently drop a fix. **Checklist (PR11545-F1 through F8):** | ID | Status | Commit | Evidence at current head (`fbd46ce70df3`) | |----|--------|--------|------| | F1 | Fixed | `504eb8b2979` | `seatunnel-core/seatunnel-starter/src/main/bin/seatunnel-cluster.sh:157-179` — launcher now fails fast on Java <11 and appends the six module flags itself (dedup'd against whatever the config file already has), so it no longer depends on the config file alone. | | F2 | Fixed | `1b1da9e864e` | `KuduUtil.java:154-158` and `PaimonSecurityContext.java:144-148` — both now catch `IllegalAccessException` separately and `log.error` the exact missing flag instead of swallowing it. | | F3 | Open | — | Not yet touched. | | F4/F7 | Fixed | `be5492399ba` | `.github/workflows/upgrade_compatibility.yml:60` — `java-version: "11"` (not 17), so the old 2.3.13 release no longer runs under a JDK that its bundled config doesn't have flags for. | | F5 | Fixed | `504eb8b2979` | Same launcher change as F1 — `seatunnel-cluster.sh:157-161` is the explicit "SeaTunnel requires Java 11 or newer" pre-flight check, replacing the bare JVM `Unrecognized option` abort. | | F6 | Open | — | Not yet touched. | | F8 | Open | — | Not yet touched (same root cause/fix as F6 — see below). | Direct answers to your specific questions: - **F1**: yes — the flags now live in the launcher scripts (`seatunnel-cluster.sh` / `seatunnel.sh`), not only in `config/jvm_*_options`. An in-place upgrade that preserves an old config directory still gets the flags, because the launcher assembles and appends them itself rather than only reading what the config file happens to contain. - **F2**: yes — both `KuduUtil` and `PaimonSecurityContext` now `log.error` with the specific missing `--add-exports`/`--add-opens` flag name when the reflective call throws `IllegalAccessException`, instead of the previous silent swallow. - **F4/F7**: resolved by sidestepping the scenario rather than adding flags — `upgrade_compatibility.yml` now pins JDK 11 for the whole job, so the old 2.3.13 release never runs under 17's stronger encapsulation in the first place. That's a narrower fix than "add the full flag set for the old release," but it avoids the false-negative risk you flagged without needing to guess which flags a Java-8-era build actually needs. - **F5**: yes — same `seatunnel-cluster.sh:157-161` check as F1. A leftover JDK 8 now gets `"Error: SeaTunnel requires Java 11 or newer, but Java 8 was detected..."` instead of the JVM's raw `Unrecognized option: --add-opens=...`. - **F3/F6/F8**: still genuinely open, no code change yet. I'm not going to claim otherwise — I flagged this same gap against myself in my last review: I said on Sept 4 I'd land these before rebasing, and then the `dev`-sync rebase went ahead without them. The intended fix is still what I described on Aug 28: scope F3 to least-privilege (drop the flags/opens that aren't actually needed per-process instead of the current blanket set on every node), and for F6/F8 move the module flags out of workflow-level `JAVA_TOOL_OPTIONS` in `backend.yml` and into a JDK9+-activated Maven profile's surefire/failsafe `argLine` in the root `pom.xml`, so CI and local `./mvnw test` runs stay in parity instead of CI silently papering over a packaging gap. I called out that the `pom.xml` side of that needs a look from whoever's driving this PR before I touch it, which is why it's sequenced as a fast-follow rather than done yet — I don't have a firm ETA to give you today, but I'll push the actual com mits (not just another status update) before asking for the rebase-then-final-pass you agreed to above. To be clear about severity, matching my own last review: F3/F6/F8 are Medium/Medium/Low, not the High-severity F1 — so I don't think they need to block a first approval the way F1 did, but that's your call as the reviewer, not something I get to decide unilaterally on my own PR. If you'd rather hold the approval until all three land, that's a completely reasonable position and I'll just get them done first. -- 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]
