DanielLeens commented on PR #11545:
URL: https://github.com/apache/seatunnel/pull/11545#issuecomment-5453630641
Thanks for continuing to dig into this, @SEZ9. Before responding
point-by-point, I re-checked the current head (`c32b89fd4b1`) against each
finding directly — five of these eight are already fixed, three commits ago
(`504eb8b2979`, `1b1da9e864e`, `be5492399ba`, all landed 2026-08-26, well
before this comment). I think this pass may have been generated against a stale
diff rather than the current head, so let me show the actual current state of
each:
**Issue 1 (High, blocking) — fixed in `504eb8b2979`.** The evidence cited
(`config/jvm_master_options:22-24`) is from before that commit. The current
`seatunnel-core/seatunnel-starter/src/main/bin/seatunnel-cluster.sh` (and
`seatunnel.sh`) now assembles the mandatory flags itself:
```
$ curl -s .../seatunnel-cluster.sh?ref=c32b89fd4b1 | grep -n 'requires Java
11\|module_flag'
157:# SeaTunnel requires Java 11 or newer. Fail fast with an actionable
message instead of letting a
159:JAVA_MAJOR_VERSION=$(java -version 2>&1 | awk -F '[".]' '/version/
{print ($2 == "1") ? $3 : $2; exit}')
160:if [[ -n "$JAVA_MAJOR_VERSION" && "$JAVA_MAJOR_VERSION" -lt 11 ]]; then
170:for module_flag in \
```
The six `--add-opens`/`--add-exports` flags are appended here with a dedupe
check against whatever the config file already carries, so a config directory
preserved across an in-place upgrade no longer causes the flags to be silently
dropped — the launcher supplies them either way.
**Issue 2 (Medium) — fixed in `1b1da9e864e`.** `KuduUtil.java` and
`PaimonSecurityContext.java` now catch `IllegalAccessException` (the
module-denial case) separately from ordinary refresh failures and `log.error`
the exact missing `--add-exports` flag:
```
$ curl -s .../KuduUtil.java?ref=c32b89fd4b1 | grep -n
'IllegalAccessException\|log.error'
154: } catch (IllegalAccessException e) {
158: log.error(
```
**Issue 4 / 7 (Medium) — fixed in `be5492399ba`.**
`upgrade_compatibility.yml` is pinned to JDK 11, not 17:
```
$ curl -s .../upgrade_compatibility.yml?ref=c32b89fd4b1 | grep -n
java-version
60: java-version: "11"
```
**Issue 5 (Medium) — also fixed by `504eb8b2979`**, same commit as Issue 1:
the "requires Java 11 or newer" pre-flight check quoted above is exactly the
graceful version check this issue asks for, replacing the raw `Unrecognized
option` launcher abort with an explicit message.
**Issues 3, 6, 8 — still open, agreed.** These are legitimate and I haven't
dismissed them: least-privilege scoping of the flags (3) and moving them into a
surefire/failsafe `argLine` via a JDK9+ Maven profile in the root `pom.xml`
instead of workflow-level `JAVA_TOOL_OPTIONS` (6, 8). The `pom.xml` change
needed for 6/8 is one I want to run past whoever's driving this from my side
before touching it, so I'm treating those two as fast-follow rather than
blocking this round — let me know if you'd rather they land before merge
instead.
Given 1/2/4/5/7 are confirmed fixed against the current head, could you take
another pass at `c32b89fd4b1` when you get a chance? Happy to be wrong if I've
mis-read something, but the source at that ref doesn't match what the finding
descriptions say is there.
--
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]
