SEZ9 commented on PR #11545: URL: https://github.com/apache/seatunnel/pull/11545#issuecomment-5976026404
Thanks for mapping each point to the commits on `ac6c41cdb` — that helps. I have not re-reviewed the diffs yet, so I'm keeping everything open until I can check them against a usable CI run. Notes per item: **F1 / F5 (`504eb8b29`)** — The described approach (enforcing the Java 11 baseline and re-injecting the mandatory module flags in `seatunnel.sh` / `seatunnel-cluster.sh`) sounds like the right direction. I'll verify the script changes on re-review. Please land the `incompatible-changes.md` note with the next push as you proposed. **F2 (`1b1da9e86`)** — Reporting denied Kerberos reflective access in `KuduUtil.java` / `PaimonSecurityContext.java` instead of failing silently is what the finding asked for; I'll confirm the behaviour in the diff. **F4 / F7** — These still look open. `1b1da9e86` is described as Kerberos error reporting, which doesn't cover the `upgrade_compatibility.yml` scenario: the OLD 2.3.13 release being started under JDK 17 with only the jgss export and no module flags in its bundled config. Could you clarify how the workflow handles the old cluster now — does it run on a JDK 8/11 runtime, pass the flags explicitly, or something else? If nothing covers it yet, please add that handling or explain why the old cluster starts cleanly as-is. **F3** — Per-flag comments naming the consumer are helpful, but the finding is about scope: the flags are unchanged and still open JDK internals to `ALL-UNNAMED` for every jar on the classpath. I can't point to a specific flag as unnecessary without evidence either way, so could you share what you have for the Hazelcast reflective access requirement (stack traces or the failing paths), and whether any of the six can be narrowed or dropped? I'd like to keep this one open rather than close it on annotation alone. **F6 / F8 (`7c91ebec2`)** — Replacing the workflow-level `JAVA_TOOL_OPTIONS` with `SUREFIRE_JVM_ARGS` plus a `surefire.module.args` property addresses both concerns if it works as described; I'll confirm in `backend.yml` and the pom. Please also include the header-comment fix you mentioned (stale `jdk9-plus-test-opens` reference) in the next push. Before I re-review: 1. Clarify / fix the old-release-on-JDK-17 scenario in `upgrade_compatibility.yml` (F4/F7). 2. Add the `incompatible-changes.md` note (F1) and the `backend.yml` comment fix (F6). 3. Share the evidence behind the F3 flag list, or a narrowed list. 4. Re-trigger the fork CI after that push and link the run here, since the current head's run isn't usable as evidence. <!-- streview-comment:1503 --> -- 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]
