DanielLeens commented on PR #11545: URL: https://github.com/apache/seatunnel/pull/11545#issuecomment-5412614642
Thanks for the thorough re-review, @SEZ9 — this is a genuinely new set of findings, not overlapping with the CI-evidence issues (hangs, transient failures) I was tracking in my own last round, so let me go through them with a fresh source check rather than just taking the list at face value. I re-verified the two claims that matter most before agreeing with anything: - **Issue 1 (High, blocking)** — confirmed. I grepped `bin/*.sh` and `config/jvm_*_options` on the current head: the six `--add-opens`/`--add-exports` lines exist *only* in `config/jvm_master_options`, `config/jvm_worker_options`, `config/jvm_client_options`, and `config/jvm_options` — nothing in the launcher scripts assembles or falls back to them. You're right that an in-place upgrade which preserves an existing config directory (a mounted Docker volume or K8s ConfigMap, as you note) silently loses every one of these flags, and the Kerberos reflective path is documented in this PR's own comments to fail silently when that happens. That's a real gap and I'm treating it as blocking, same as you. - **Issue 6/7 (Medium)** — also confirmed. `backend.yml:37` sets the full six-flag `JAVA_TOOL_OPTIONS` at workflow scope, applying to every JVM in every job, while `upgrade_compatibility.yml:49` sets only the single krb5 `--add-exports` for the JDK-17 runner that boots the old 2.3.13 release (`java-version: "17"` at line 56) — which predates all six flags in its own bundled config. Both checks out exactly as you describe: CI can currently mask a packaging gap that would bite a real user, and the upgrade-compatibility job's flag set doesn't match what an old-config-carrying node under JDK 17 actually needs. I haven't independently re-verified every one of your Medium/Low items (3, 5, 8) line-for-line yet, but nothing in a quick read of them looks off, and none conflict with anything I'd previously confirmed. Net: Issue 1 supersedes my own last round's Issue 5 (CI-hang reruns) as the primary blocker — the hang question still needs a clean rerun before merge, but this is a correctness gap in the shipped fix itself, not a CI-evidence question, so it has to be closed first. I'll push a fix moving the mandatory flags into the launcher scripts' JVM argument assembly (so they survive config overrides) and revisit the CI-scoping items in the same round, then do a full fresh pass. As before: I'm the PR author here too, so this is not an independent approval — a write-capable maintainer should still do the final sign-off. -- 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]
