SEZ9 commented on PR #11545: URL: https://github.com/apache/seatunnel/pull/11545#issuecomment-5673812557
Thanks for the update. I've looked at the new head `81f12fffbd8` alongside the automated re-review, which confirms it is a plain `Merge branch 'dev'` on top of `b75d9123be` with no content change to this PR's own diff (only shifted context in `docs/en/introduction/concepts/incompatible-changes.md` and its `zh` counterpart). So the status of the previous findings is unchanged by this commit; here is where I think each one stands and what I still need from you. **Looks addressed per the automated summary – please confirm in the description** - **PR11545-F1 (module flags only in user-overridable `config/jvm_*_options`)** – the summary states the `--add-opens`/`--add-exports` flags are now injected idempotently from the launcher scripts so a preserved config directory can't silently drop them. Please spell out in the PR description how the idempotency check works (what the script keys on, and what happens if a user has already hand-added a subset of the flags) so we can verify it rather than infer it. - **PR11545-F5 (cryptic `Unrecognized option` on leftover JDK 8)** – the summary mentions a fail-fast JDK-version pre-check in the launcher scripts. Please state the minimum version it enforces and the exact user-facing message, so I can check it against the `incompatible-changes.md` entry. **Still open – need a response or a change** - **PR11545-F2 (silent Kerberos reflective-reload failure when flags are missing)** – nothing in the thread shows a runtime detection/report for this condition. If the launcher-script injection is meant to make it unreachable, please say so explicitly and note what happens for users who start the JVM without the scripts (e.g. embedded/custom launchers). - **PR11545-F3 (blanket `--add-opens`/`--add-exports` to `ALL-UNNAMED`)** – please list which flags are required by which component (Kerberos vs Hazelcast vs Hadoop/ORC) and drop any that are not strictly needed, or explain why the blanket set is unavoidable. - **PR11545-F4 / PR11545-F7 (`upgrade_compatibility.yml` runs the old 2.3.13 release on JDK 17 with only the jgss export)** – please clarify whether the old cluster is intended to run on JDK 17 at all. If yes, explain why the jgss export alone is sufficient for that release's bundled config; if no, run the old side on JDK 11 so the scenario reflects a realistic upgrade path. - **PR11545-F6 / PR11545-F8 (workflow-level `JAVA_TOOL_OPTIONS` in `backend.yml`)** – as far as the thread shows this is still global. Please either scope it to the specific jobs/steps that need it or explain why CI on a plain JDK 11/17 without injected flags is not a scenario we need to prove; the `Picked up JAVA_TOOL_OPTIONS` stderr noise would also go away with the narrower scope. Once the description covers F1/F5 and you've replied to F2/F3/F4/F6/F7/F8 (with a follow-up commit where applicable), I'll do a final pass. No new topics from my side – this is just closing out the existing list. <!-- streview-comment:1053 --> -- 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]
