DanielLeens commented on PR #11545:
URL: https://github.com/apache/seatunnel/pull/11545#issuecomment-5678788648
Re-review at the new head (`00794fa12ee932c611dfad4d77c9b41178438c1d`), own
PR, reviewing as maintainer under the same rigor as any other contribution
(GitHub blocks self-approval, so this is posted as a comment, not a formal
review).
**Procedural note first:** the new commit is a plain `Merge branch 'dev'
into dev-ci-jdk17-20260723`. I diffed it directly against the
previously-reviewed head (`81f12fffbd8`) rather than assuming: `gh api
repos/apache/seatunnel/compare/81f12fffbd...00794fa12e` shows 15 changed files,
all unrelated `dev`-side changes pulled in by the merge (RabbitMQ connector
docs/source/sink/tests, one S3 dry-run E2E test, `incompatible-changes.md`
context-line shifts) — none of them touch anything this PR's own diff owns (no
`pom.xml`, no `config/jvm_*_options`, no launcher scripts, no CI workflow
files). So this round is content-identical to `81f12fffbd8` for everything this
PR is actually responsible for, and I re-verified that directly in the
checked-out source rather than trusting the merge commit message alone.
# What Problem Does This PR Solve?
Retires Java 8 as SeaTunnel's build and runtime baseline: the Maven compiler
target moves to Java 11 bytecode (`maven.compiler.release`, not just
`-source`/`-target`), the CI matrix moves from JDK 8/11 to JDK 11/17, launcher
scripts fail fast with an actionable message on a sub-11 JDK, and the handful
of JDK 9+ module-system flags (`--add-opens`/`--add-exports`) this project's
Hazelcast/classloader/Kerberos-reflection code needs are threaded through
consistently — JVM runtime flags via `config/jvm_*_options` plus idempotent
launcher-script injection, and JDK-9+-gated `javac` compiler flags via new
Maven profiles for the two modules (`connector-kudu`, `connector-paimon`) that
reflectively touch encapsulated `sun.*` packages.
**One-sentence summary:** this round changes nothing about that story — it's
a pure `dev` sync — so the one substantive item still open from my last full
review (`81f12fffbd8`) is still open, and everything else I previously verified
as correct remains correct because nothing touching it has changed.
# 1. Code Change Review
## 1.1 Core Logic Analysis
Since this round's diff (restricted to this PR's own files) is empty, I
re-verified the one still-open item directly in the current checked-out source
rather than re-describing the full history:
**Issue 1 (carried over, still open): dead compile-time krb5 export flag
with a now-inaccurate comment in `connector-kudu`/`connector-paimon`.**
Both `KuduUtil.java` and `PaimonSecurityContext.java`, at the current head,
resolve `sun.security.krb5.Config` via reflection, not a direct import:
```java
// KuduUtil.java:153
Class.forName("sun.security.krb5.Config").getMethod("refresh").invoke(null);
```
```java
// PaimonSecurityContext.java (same pattern)
```
I grepped both files for any `import sun.security.krb5...` line — there is
none in either. Yet both POMs still carry, unchanged since my last review:
```xml
<!-- connector-kudu/pom.xml:75, connector-paimon/pom.xml:134 -->
KuduUtil directly imports sun.security.krb5.Config/KrbException to ...
PaimonSecurityContext directly imports sun.security.krb5.Config/KrbException
to ...
...
<arg>--add-exports=java.security.jgss/sun.security.krb5=ALL-UNNAMED</arg>
```
A **direct import** is what forces `javac` itself to need `--add-exports` (a
module-system *compile-time* check on referencing an encapsulated package by
name); `Class.forName(...)` + reflection is exactly the pattern this PR uses
everywhere else specifically to *avoid* that compile-time requirement — the two
are inconsistent within this same PR. The compile-time flag is not actively
harmful (an unnecessary `--add-exports` at compile time is a no-op, not an
error), but the comment describing *why* it's there is factually wrong about
the current source, which is exactly the kind of self-inconsistency this PR's
own stated goal (precision about which JDK 9+ flags are needed and why)
shouldn't leave behind. Only `connector-kudu`'s `sun.misc` export (for the
test-only `Unsafe` import) is genuinely load-bearing.
**Everything else** — the F1-F8 items from the back-and-forth with @SEZ9
that I went through point-by-point in my last comment (idempotent flag
injection in the launcher scripts, the already-explicit `ERROR`-level logging
on a denied Kerberos reflective reload, per-flag attribution comments in
`config/jvm_options`, the `upgrade_compatibility.yml` premise clarification,
and `JAVA_TOOL_OPTIONS` removal from `backend.yml`) — is unchanged by this
round's merge-only commit, and I'm not re-litigating already-closed items here;
see that comment for the full mechanics.
## 1.2 Compatibility Impact
**Partially incompatible, disclosed** — unchanged from every prior round:
retiring JDK 8 as a supported runtime is an intentional, documented breaking
change (`docs/en`/`docs/zh` `incompatible-changes.md`), not a side effect.
## 1.3 Performance / Side-Effect Analysis
No change. The runtime-side flags are a handful of
`--add-opens`/`--add-exports` entries with no measurable JVM cost; the
JDK-9+-gated compiler profiles only affect `javac` invocation at build time.
## 1.4 Error Handling and Logging
No change. The `ERROR`-level log on a denied Kerberos reflective reload
(`KuduUtil`/`PaimonSecurityContext`) remains the real safety net for a user
running a custom launcher that doesn't inject the runtime export flag.
# 2. Code Quality Assessment
## 2.1 Coding Standards
No new code this round. Standing issue is documentation accuracy (Issue 1),
not style.
## 2.2 Test Coverage and Test Stability
No new test code this round (pure `dev` merge). Re-confirming the one
still-open, previously-flagged item:
**Stability rating: Risk present.**
`seatunnel-e2e/seatunnel-connector-v2-e2e/connector-python-e2e/.../PythonIT.java:78-101`
(`installPythonIfNecessary`, `@BeforeAll`) execs `apt-get`/`dnf`/`yum`/`apk
install -y python3` inside the running container at test-startup time,
network-dependent, no retry. A transient package-mirror/network hiccup would
fail this test for reasons unrelated to the Python connector itself and would
read as a confusing, unrelated CI flake to whoever triages it later. Not a hard
sleep or an ordering race, so not High, but worth fixing (bake `python3` into
the image, or wrap the install in retries with backoff). Carried forward from
the prior round, not new.
## 2.3 Documentation Updates
`docs/en`/`docs/zh` `incompatible-changes.md` remain updated and consistent;
this round only shifted context lines via the merge, no new content.
# 3. Architectural Soundness
## 3.1 Elegance of the Solution
**Precise fix** for the stated JDK-8-gating goal. Still short of fully
precise only on Issue 1 — a compile-time flag re-added to protect against a
dependency pattern (direct import) that no longer exists in the source it's
commented as protecting.
## 3.2 Maintainability
Good once Issue 1 is closed — at that point the only compiler-level module
flag left in either POM is the one that's actually load-bearing (`sun.misc`,
Kudu's test sources).
## 3.3 Extensibility
The `jdk9-plus-*` profile pattern is easy to replicate for any future module
that gains a genuine JDK 9+-only compile-time dependency.
## 3.4 Historical-Version Compatibility
No change from prior rounds' assessment.
# 4. Issue Summary
| # | Issue | Location | Severity | Raised by another reviewer |
|---|-------|----------|----------|------|
| 1 | `connector-kudu`/`connector-paimon`'s `jdk9-plus-*-compile-exports`
profiles still carry a dead
`--add-exports=java.security.jgss/sun.security.krb5=ALL-UNNAMED`, with comments
claiming a "direct import" of `sun.security.krb5.*` that doesn't exist in
either module's source (both use `Class.forName(...)` reflection) |
`seatunnel-connectors-v2/connector-kudu/pom.xml:75,102`;
`seatunnel-connectors-v2/connector-paimon/pom.xml:134,160` | Medium | No |
| 2 | `PythonIT.installPythonIfNecessary()` runs a network-dependent
package-manager install inside the container during `@BeforeAll` with no retry
— a plausible source of an intermittent, hard-to-triage CI flake unrelated to
the Python connector |
`seatunnel-e2e/seatunnel-connector-v2-e2e/connector-python-e2e/.../PythonIT.java:78-101`
| Medium | No |
Both carried over unchanged from my last full review — nothing new this
round, nothing resolved either.
# 5. Merge Recommendation
### Conclusion: Ready to merge after fixes
1. **Blockers — must be fixed:** Issue 1. Since this PR's own stated purpose
is precision about which JDK 9+ module flags are actually needed and why, I'd
like to close this out before merge: drop the dead krb5 `--add-exports` line
from both `jdk9-plus-kudu-compile-exports` and
`jdk9-plus-paimon-compile-exports`, keep only Kudu's genuinely-needed
`sun.misc` export, and fix the comments so they stop describing a "direct
import" that no longer exists in either module.
2. **Recommended fixes — non-blocking:** Issue 2 (harden `PythonIT`'s
runtime package install, or move it to image-build time) — real, but a
pre-existing E2E-stability concern independent of this PR's core goal; fine as
a fast follow-up.
**CI status:** apache-side `Build` is `IN_PROGRESS` on this exact head
(`00794fa12e`) at review time — no fresh pass/fail signal yet. Given this
round's only change is the `dev`-merge noise described above, and prior runs on
this branch have been green modulo the already-tracked, unrelated environmental
flakes from earlier rounds, I don't expect anything new to surface, but I'm not
claiming a result that hasn't landed.
**Upstream sync:** current as of this exact head — this round's own commit
*is* the `dev` sync, so there's no separate upstream-sync blocker to track.
Overall assessment: unchanged from my last full review — a careful,
well-targeted, and by now very thoroughly discussed fix for a real JDK 8/9+
gap, with one self-consistency cleanup (Issue 1) standing between this and a
clean "ready to merge." Thanks again to @SEZ9 for the sustained, detailed
back-and-forth across this PR's review history — it's meaningfully improved the
precision of the final flag set and its documentation.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
