DanielLeens commented on PR #11545:
URL: https://github.com/apache/seatunnel/pull/11545#issuecomment-5394075927
# What Problem Does This PR Solve?
- User pain point: GitHub CI validated SeaTunnel almost exclusively on JDK
8/11 toolchains, so Java 17 module-system restrictions, internal-API removals,
and reflective-access changes stayed invisible until users hit them in
production. Separately, the shipped Docker image and `config/jvm_*_options`
never carried the `--add-opens`/`--add-exports` flags CI itself needs to pass
on newer JDKs.
- Fix approach: move the CI matrix from JDK 8/11 to JDK 11/17, raise the
Maven compiler baseline to Java 11 via `maven.compiler.release` (ten modules
pinned back to `release=8` where they must still emit Java 8 bytecode for older
dependencies), fix every JDK-17 incompatibility CI then surfaced, and carry the
runtime flags plus a JDK-11 Docker image into the actually-shipped product and
every E2E engine image.
- One-sentence summary: this is round 18 of review on an unusually deep,
well-converged PR; the two commits landed since the last full review fix the
sole remaining High-severity blocker (a self-caught compile error plus a
leftover JDK-8-matrix straggler), and I independently confirmed both fixes are
working against a live, still-in-progress CI run rather than trusting the diff
alone.
**Housekeeping note.** Sorry for the one gap in the last 17 rounds'
otherwise very thorough documentation sweep — this round's fresh full-diff pass
(not incremental) found `docs/{en,zh}/developer/shade-guide.md` still says
"Java 8+" as a prerequisite, contradicting the Java 11 baseline this PR itself
documents everywhere else. That's a genuine miss in a check ("does every doc
location agree on the new minimum") that prior rounds explicitly claimed to
have completed. See Issue 6.
# 1. Code Change Review
## 1.1 Core Logic Analysis
**The two commits since the last full review (2026-08-23T11:28Z),
independently read from the current head `51d884bfb`:**
1. `2cec73da1` (`[Fix][E2E] Use the inherited server field in PythonIT`) —
one-line fix: `PythonIT.installPythonIfNecessary()` called
`container.execInContainer(...)`, but `PythonIT extends SeaTunnelContainer`,
which has no field named `container` (the real field is `protected
GenericContainer<?> server`). This was a genuine compile break in the commit
the *previous* review round had endorsed by reading, not by a completed compile
— the fork's queued CI run for that head hadn't finished when that round was
written. Daniel self-caught this via a later completed CI run and posted a
self-correcting issue comment before this round started; this commit is exactly
that fix, and it is correct (`server.execInContainer(...)` matches the actual
inherited field).
2. `51d884bfb` (`[Fix][CI] Migrate google-pubsub-connector-it to the JDK
11/17 matrix`) — the `google-pubsub-connector-it` job in `backend.yml` had been
left on `java: [ '8', '11' ]` while every other job in the file was already
converted to `[ '11', '17' ]`. This commit converts it and adds `fail-fast:
false`, matching the file's established per-job convention. Independently
verified: `git show pr-11545:.github/workflows/backend.yml | grep -c "java: \[
'11', '17' \]"` and a search for any remaining `'8'` matrix entry both confirm
zero stragglers now — the whole file is internally consistent.
**Independent full-diff re-sweep this round (not incremental), focused on
the areas the task brief called out:**
- **CI workflow consistency** — read every `.github/workflows/*.yml` file's
Java references in full, not just the diff hunks. `backend.yml`,
`benchmarks.yml`, `codeql.yaml`, `publish-docker.yaml`,
`upgrade_compatibility.yml` are all uniformly on `11`/`17`. `build_main.yml`,
`schedule_backend.yml`, `publish-e2e-spark-images.yaml`,
`notify_test_workflow.yml`, `update_build_status.yml` have no Java version
references at all (orchestration-only), so they correctly need no change. No
stale JDK 8 matrix entry survives anywhere in `.github/workflows/`.
- **Downstream-compatibility for older Flink/Spark consumers** — correctly
and explicitly documented in `incompatible-changes.md`: "Third-party connectors
that are themselves compiled for Java 8 keep working. A Java 11 JVM loads older
class files without changes, so only the JVM version matters." The ten
`release=8`-pinned modules (`connector-cdc-tidb-e2e`, `connector-fluss-e2e`,
`connector-tdengine-e2e`, `connector-typesense-e2e`, `seatunnel-flink-examples`
+ its 3 sub-modules, `seatunnel-format-protobuf`, `seatunnel-ci-tools`) all
already carried explicit `<source>8</source>`/`<target>8</target>` **before**
this PR (verified via the diff hunks — only the `<release>8</release>` line is
new in each), so this PR is faithfully carrying forward pre-existing,
intentional pins under the new `release`-based mechanism, not inventing new
ones. No shipped production connector module is left targeting Java 8 bytecode;
only test/example modules with known old-dependency constraints are p
inned.
- **`connector-fluss`'s new `commons-lang3` pin + relocation** — traced the
actual bug: `fluss-client` calls `SystemUtils.isJavaVersionAtLeast`, which NPEs
on `JavaVersion.get(...)` returning `null` for JDK 11+ against an old
`commons-lang3`. The real-world trigger named in the code comment is Spark 2.4
putting its own old `commons-lang3` (3.5) on the classpath ahead of whatever
this module bundles — that's why relocation (not just a version bump) is
necessary: it guarantees `fluss-client`'s call resolves against the connector's
own bundled (and patched-in-place) copy regardless of classloader ordering.
`${commons-lang3.version}` resolves to `3.18.0` (well past the
`JavaVersion.JAVA_17` constant's introduction), so the fix is durable, not just
past-JDK-11-only. The new `<build>` shade block with no `<artifactSet>`
restriction matches the established pattern used by sibling connectors (e.g.
`connector-google-pubsub`'s shade config), so it's not a deviation from
convention — t
hough see Issue 7 for one small gap relative to that same sibling pattern.
Runtime path this PR changes (build/CI/deploy plane only; no job-execution
data path is touched):
```text
GitHub Actions matrix (backend.yml, CodeQL, publish-docker,
upgrade-compatibility, benchmarks.yml)
-> JDK 11 / JDK 17 toolchains (was 8 / 11), now with zero stale JDK-8
stragglers (this round's fix)
-> mvnw compile with maven.compiler.release=11 (10 modules pinned to
release=8, pre-existing pins carried forward)
-> seatunnel-dist/src/main/docker/Dockerfile: eclipse-temurin:11-jdk +
apt-get install python3
-> config/jvm_{master,worker,client}_options, jvm_options: carry
--add-opens/--add-exports
-> (separately) SeaTunnelContainer.JDK_DOCKER_IMAGE =
eclipse-temurin:11-jdk,
python3 installed at test runtime via installPythonIfNecessary() (now
compiles; confirmed green — see 1.4)
```
## 1.2 Compatibility Impact
**Partially incompatible, correctly documented.** Raising the minimum Java
runtime from 8 to 11 is a breaking change for any deployment still running the
SeaTunnel process (or a Flink/Spark cluster it submits to) on Java 8, captured
in both `docs/en` and `docs/zh` `introduction/concepts/incompatible-changes.md`
with impact and a migration guide. No SeaTunnel config `Option`, public API,
job-file format, or checkpoint/savepoint format is touched. The one gap found
this round is a stale prerequisite statement in a developer-facing doc (Issue
6), not a functional compatibility gap.
## 1.3 Performance / Side-Effect Analysis
No hot-path/runtime performance impact; this is build, CI, and deployment
tooling only. The one previously-identified side effect — `PythonIT`'s
`installPythonIfNecessary()` doing a live `apt-get install` inside the test
container at `@BeforeAll` time, a new network dependency the old pre-baked base
image didn't have — is unchanged by this round's fix and remains worth watching
for CI flakiness, though it isn't new since the last review.
## 1.4 Error Handling and Logging — Live CI Verification and Issue List
I did not stop at reading the diff. I pulled the current state of the fork's
live Actions run for this exact head (`DanielLeens/seatunnel` run
`32697371689`, still `in_progress` at review time) to confirm the two delta
commits actually work, not just read correctly:
- `google-pubsub-connector-it (11, ubuntu-latest)` and `(17,
ubuntu-latest)`: both **success** — confirms the matrix-migration fix (commit
2) is not just syntactically consistent but functionally correct.
- `all-connectors-it-5 (11, ubuntu-latest)` (the bucket containing
`PythonIT`): **success** — confirms the `server` field fix (commit 1) compiles
and the Python E2E test passes end-to-end on JDK 11 with the new
install-at-runtime `python3` step.
- `all-connectors-it-5 (17, ubuntu-latest)`: still `in_progress` at review
time — not yet confirmed, but there is no structural reason to expect
JDK-17-specific divergence given the JDK-11 lane is clean and both
`google-pubsub` lanes (11 and 17) already match.
- `all-connectors-it-1 (17, ubuntu-latest)`: **failure**, isolated to
`connector-couchbase-e2e`. Pulled the job log directly — this is the
previously-tracked `CouchbaseIT` container-bootstrap flake (independently fixed
by PR #11845, unrelated to this PR's JDK changes). Matches Issue 2's carryover
status exactly.
- `rocketmq-connector-it (11, ubuntu-latest)`: **failure**, new observation
this round. Pulled the job log: `RocketMqIT.testSourceRocketMqRestore` failed
with `AssertionFailedError: Unexpected sink message count after restore.
Expected: 45, actual: 46` after a 53-minute run through the full 87-test suite.
This is an off-by-one message-count assertion in a checkpoint-restore test —
the classic signature of an at-least-once redelivery timing flake, not a
JDK-bytecode or module-system issue. Nothing in this PR's diff touches
`connector-rocketmq` or its E2E test. See Issue 8.
- `kudu-connector-it (11 and 17, ubuntu-latest)`: both `cancelled` —
fail-fast fallout from the Couchbase/RocketMQ failures elsewhere in the same
run, not independent findings.
**Formal issue list** (carryover items re-verified directly against current
source this round, not assumed unchanged; new items marked accordingly):
**Issue 1 (Medium, downgraded from High — code-level fix confirmed working,
CI still finishing) — Docker/E2E `python3` gap fix now live-verified**
- Location: `seatunnel-dist/src/main/docker/Dockerfile:15-17`;
`seatunnel-e2e/.../PythonIT.java`.
- Problem: was the sole confirmed blocker from the last full round. Both the
shipped-image fix and the E2E-harness fix are now committed.
- Potential risk: the JDK 17 lane of `all-connectors-it-5` had not finished
at review time, so full confirmation across both JDKs is still pending, though
the JDK 11 lane and both `google-pubsub` lanes are already green with no sign
of JDK-version-specific divergence.
- Best improvement: none needed if the JDK 17 lane finishes green, which is
expected.
- Severity: Medium (verification gap, not a known code defect). Raised by
another reviewer: Yes (@SEZ9), carryover, now materially advanced by this
round's live CI check.
**Issue 2 (Low, carryover, re-confirmed today) — `CouchbaseIT` failure on
the JDK 17 lane, unrelated to this PR**
- Location: `all-connectors-it-1 (17, ubuntu-latest)` (bucket assignment
shifts run to run; same underlying flake).
- Problem: confirmed again this round via a fresh log pull — same
container-bootstrap-timeout signature tracked by PR #11845, independent of this
PR's Dockerfile/JDK changes.
- Severity: Low. Raised by another reviewer: Yes (@SEZ9).
**Issue 3 (Medium, carryover, still open) — `upgrade_compatibility.yml` runs
on JDK 17 against released `2.3.13` artifacts with no independent confirmation
yet that the upgrade path itself works on the new baseline**
- Location: `.github/workflows/upgrade_compatibility.yml`.
- Severity: Medium. Raised by another reviewer: No.
**Issue 4 (Medium, carryover, still open) — No Maven Enforcer
`requireJavaVersion` rule for a contributor still on JDK 8**
- Location: root `pom.xml` (absent). Re-verified: still no
`maven-enforcer-plugin` `requireJavaVersion` rule in the current `pom.xml`.
- Severity: Medium, non-blocking quality-of-life. Raised by another
reviewer: No.
**Issue 5 (Medium, carryover, still open) — Kudu/Paimon Kerberos reflection
shims diverge in failure semantics**
- Location: `KuduUtil.java:140-160` vs `PaimonSecurityContext.java:139-160`.
- Problem: both replace the now-non-compiling `sun.security.krb5.Config`
import with `Class.forName(...).getMethod("refresh").invoke(null)`, correctly
(verified: `refresh()` is public static, so `--add-exports` alone is sufficient
and the JDK-17-vs-11 relaxed-encapsulation difference does matter here —
re-verified this is a genuine requirement, not defensive). But `KuduUtil`'s
helper swallows `ReflectiveOperationException | RuntimeException` internally
and moves `KerberosName.resetDefaultRealm()` outside the try, so it now runs
unconditionally even after a failed refresh — a real behavior change from the
pre-PR code (which skipped `resetDefaultRealm()` on failure) and from
`PaimonSecurityContext`'s otherwise-identical fix in this same PR (which
correctly preserves the skip-on-failure ordering). Neither shim unwraps
`InvocationTargetException`, so a genuine `KrbException` from a malformed
`krb5.conf` is logged as a generic reflection failure.
- Best improvement: make `KuduUtil` match `PaimonSecurityContext`'s ordering
(propagate, skip `resetDefaultRealm()` on failure), or state explicitly that
the divergence is deliberate; unwrap `InvocationTargetException` in both.
- Severity: Medium, non-blocking (Hadoop's `resetDefaultRealm()` is itself
internally exception-safe in practice, so the real-world blast radius is small,
but it is an unflagged semantic change touching Kerberos-secured cluster
behavior). Raised by another reviewer: No (this is Daniel's own carryover
finding from prior rounds, re-verified unchanged against current source this
round).
**Issue 6 (Low, NEW this round) — `docs/{en,zh}/developer/shade-guide.md`
still lists "Java 8+" as a prerequisite**
- Location: `docs/en/developer/shade-guide.md:88`;
`docs/zh/developer/shade-guide.md:88`.
- Problem: unlike `docs/{en,zh}/developer/setup.md`, which this PR correctly
updated to "JDK11/JDK17 are supported by now", `shade-guide.md`'s "Java 8+"
prerequisite line was not touched. It isn't strictly false in isolation ("8+"
technically includes 11/17), but it directly contradicts this same PR's own
`incompatible-changes.md` entry stating the minimum is now Java 11, and could
lead a contributor to set up a Java 8 toolchain that then fails to build
(`maven.compiler.release=11` cannot be satisfied by a JDK 8 `javac`).
- Potential risk: low — following the stale doc produces an immediate, clear
Maven build error rather than a silent misbehavior, so this is a
documentation-quality gap rather than a functional risk.
- Best improvement: change both files' "Java 8+" line to match `setup.md`'s
wording.
- Severity: Low. Raised by another reviewer: No — this is a genuine gap in
this round's own full sweep of every doc location; the prior 17 rounds'
documentation checks did not catch it.
**Issue 7 (Low, NEW this round) — `connector-fluss`'s new shade block omits
the `META-INF/*.SF|.DSA|.RSA` filter its sibling connectors use**
- Location: `seatunnel-connectors-v2/connector-fluss/pom.xml` (new `<build>`
block).
- Problem: `connector-google-pubsub/pom.xml` (and others) include a
`<filters><filter><artifact>*:*</artifact><excludes>` block dropping signature
files to avoid "Invalid signature file digest" errors when shading signed jars
into an uber jar. `connector-fluss`'s otherwise-identical new shade block lacks
it.
- Potential risk: low unless one of `fluss-client`'s transitive dependencies
turns out to be signed; not proven to be a live problem, just a gap versus this
codebase's own established defensive pattern for the same plugin usage.
- Best improvement: add the same filter block for consistency, at the
author's discretion.
- Severity: Low. Raised by another reviewer: No.
**Issue 8 (Low, NEW this round) — `RocketMqIT.testSourceRocketMqRestore`
failed on the live CI run, likely a pre-existing flake unrelated to this PR**
- Location: `rocketmq-connector-it (11, ubuntu-latest)`, run `32697371689`.
- Problem: `AssertionFailedError: Unexpected sink message count after
restore. Expected: 45, actual: 46` — an off-by-one message-count mismatch in a
checkpoint-restore assertion, the standard signature of an at-least-once
redelivery timing race. Nothing in this PR touches `connector-rocketmq` or its
E2E test.
- Best improvement: re-run to confirm it's non-deterministic and unrelated
to this PR; if it reproduces consistently on this exact head, escalate
separately.
- Severity: Low, not believed to be PR-caused. Raised by another reviewer:
No.
## 2. Code Quality Assessment
### 2.1 Coding Standards
Primarily CI/build-configuration, Dockerfile, and reflection-shim changes.
The new `installPythonIfNecessary()` and `EngineImageJdkUpgrader` carry
explanatory Javadoc. No new core method or non-trivial field lacks an
explanation. `KuduUtil.refreshJdkKerberosConfig()`'s Javadoc correctly explains
why reflection is needed; it does not currently mention the ordering change
flagged in Issue 5.
### 2.2 Test Coverage and Test Stability
**Stability rating: Risk present.** Not because of a flaky pattern in this
PR's own new test code, but because: (a) the JDK 17 lane of the Python E2E fix
is still finishing at review time, (b) the live `apt-get`-at-test-time step in
`PythonIT` is a genuinely new potential source of setup flakiness versus the
old pre-baked image (pre-existing observation, unchanged this round), and (c)
`RocketMqIT`'s restore-count flake (Issue 8) surfaced on this exact run,
independent of this PR.
`ServerExecuteCommandTest.testSupportedJavaVersionCheck()` correctly asserts
the new `isUnsupportedJavaVersion()` on its only reachable (`false`) branch —
the `true` branch is structurally dead code post-this-PR (already a
well-documented carryover finding across prior rounds, unchanged and
non-blocking).
### 2.3 Documentation Updates
`docs/{en,zh}/introduction/concepts/incompatible-changes.md`, `docker.md`,
`kubernetes.mdx`, and `developer/setup.md` are all consistent with the Java 11
baseline and the `11-jdk` image. `docs/{en,zh}/developer/shade-guide.md` is the
one location this round found still stale (Issue 6).
# 3. Architectural Soundness
## 3.1 Elegance of the Solution
Using `maven.compiler.release` instead of relying on `-source`/`-target`
alone remains the correct fix for its target problem (constraining the API
surface, not just the bytecode level), and pinning only the ten modules that
must stay on Java 8 bytecode — all pre-existing pins carried forward, not new
ones invented — is precise and minimal. The two-part `python3` fix (shipped
Dockerfile + independent E2E-harness base image) correctly addresses two
genuinely separate code paths that share a root cause but no build step, as
traced in the prior round and re-confirmed here.
## 3.2 Maintainability
The CI matrix replacement (8/11 → 11/17, not additive) keeps CI leg count
unchanged, and this round's fix closes the last stale-matrix straggler, so the
whole workflow file is now internally uniform — a real maintainability win,
since a future contributor scanning `backend.yml` will no longer find one job
silently still exercising an unrelated toolchain pair.
## 3.3 Extensibility
The `EngineImageJdkUpgrader` derived-build pattern for Flink/Spark E2E
images remains a reasonable, reusable approach for future JDK bumps of those
specific images. `SeaTunnelContainer`'s direct-pull-and-copy-binaries pattern,
by contrast, has no equivalent build-time hook, so any future missing-tool gap
in that image will again require a runtime `execInContainer` install step (as
this PR's own fix demonstrates) rather than a Dockerfile change — worth keeping
in mind for the next base-image swap.
## 3.4 Historical-Version Compatibility
No SeaTunnel job-file, config `Option`, checkpoint, or savepoint format is
touched by this PR. The compatibility surface is entirely "what JVM/toolchain
does the SeaTunnel process itself, and the Docker image that ships it,
require," and that surface is correctly and thoroughly documented as an
incompatible change, with the one stale-doc exception in Issue 6.
# 4. Issue Summary
| # | Issue | Location | Severity |
|---|-------|----------|----------|
| 1 | `python3` fix now live-verified on JDK 11 and both `google-pubsub`
lanes; JDK 17 Python lane still finishing | Dockerfile:15-17; PythonIT.java |
Medium |
| 2 | `CouchbaseIT` failure on 17/ubuntu lane, re-confirmed unrelated to
this PR | all-connectors-it-1 (17, ubuntu-latest) | Low |
| 3 | `upgrade_compatibility.yml`'s JDK-17 run not yet independently
confirmed green | .github/workflows/upgrade_compatibility.yml | Medium |
| 4 | No Maven Enforcer `requireJavaVersion` rule for JDK 8 contributors |
pom.xml | Medium |
| 5 | Kudu/Paimon Kerberos shims diverge in failure ordering
(`resetDefaultRealm` now unconditional in Kudu only) | KuduUtil.java:140-160;
PaimonSecurityContext.java:139-160 | Medium |
| 6 | `shade-guide.md` (en+zh) still says "Java 8+", contradicting the new
Java 11 baseline | docs/{en,zh}/developer/shade-guide.md:88 | Low |
| 7 | `connector-fluss`'s new shade block omits the signature-file filter
its sibling connectors use | connector-fluss/pom.xml | Low |
| 8 | `RocketMqIT.testSourceRocketMqRestore` failed on this run — likely
unrelated pre-existing flake | rocketmq-connector-it (11, ubuntu-latest) | Low |
# 5. Merge Recommendation
### Conclusion: Ready to merge after fixes
**1. Blockers — must be fixed / confirmed**
1. Issue 1 — obtain a completed, green run of the JDK 17
`all-connectors-it-5` lane (Python E2E) for this head or a subsequent one. The
JDK 11 lane and both `google-pubsub` lanes are already confirmed green with the
same fix, so this is a verification step rather than an expected code problem.
2. Triage Issues 2 and 8 (Couchbase, RocketMQ) as the pre-existing/unrelated
flakes the evidence points to — re-run and confirm neither reproduces
deterministically against this exact head before treating the run as clean.
**2. Recommended fixes — non-blocking**
1. Issue 3 — get at least one confirmed-green run of
`upgrade_compatibility.yml` on the new matrix.
2. Issue 4 — add a Maven Enforcer `requireJavaVersion` rule.
3. Issue 5 — align `KuduUtil`'s Kerberos-refresh failure ordering with
`PaimonSecurityContext`'s (or document the divergence as deliberate), and
unwrap `InvocationTargetException` in both.
4. Issue 6 — update `shade-guide.md` (en+zh) to state the Java 11 baseline.
5. Issue 7 — add the signature-file shade filter to `connector-fluss` for
consistency with sibling connectors.
**Overall assessment.** This PR continues to do exactly what a
JDK-baseline-raise PR should: move CI forward first, fix each incompatibility
narrowly, and close the loop into the shipped Docker image, JVM option files,
and every E2E engine image. This round's two commits close the last confirmed
code-level gap (a self-caught compile error and a leftover JDK-8 CI straggler),
and — unlike the prior round, which could only read the fix and wait on a stuck
`queued` run — I pulled the live, in-progress fork run directly and confirmed
both fixes are actually working (JDK 11 Python lane green, both `google-pubsub`
lanes green), not just plausible on paper. The two live failures on this run
(Couchbase, RocketMQ) both carry signatures pointing away from this PR rather
than toward it. The remaining open items are all Medium-or-lower, non-blocking
quality/consistency gaps accumulated and correctly tracked across 17 prior
rounds, plus two new small findings from this round's own fresh ful
l-diff and full-doc sweep (Issues 6 and 7). As the PR author is also the
account posting this review, this is not an independent approval — a
write-capable maintainer must still perform the final review, and should wait
for the JDK 17 Python lane and a clean re-run of the two flake candidates
before merging.
--
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]
