DanielLeens commented on PR #11545:
URL: https://github.com/apache/seatunnel/pull/11545#issuecomment-5340776243
# 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 (enforced via `maven.compiler.release`, not
just `-source/-target`), the CI matrix moves from JDK 8/11 to JDK 11/17, and
the shipped Docker image / `config/jvm_*_options` are updated to match. This is
a self-review — I am the PR author — read it with the same skepticism you'd
apply to anyone else's work; the merge decision still rests with a
write-capable maintainer.
**No new commit has landed since my previous review** (review id
`4968400692`, submitted 2026-08-19T04:26:46Z, reviewing this exact head
`b16d233d8469`). I re-verified that directly rather than assuming it: `git log
--oneline HEAD..apache/dev` and the fork's Actions API both confirm the head
SHA on `dev-ci-jdk17-20260723` is unchanged, and no new `DanielLeens/seatunnel`
"Build" run has been triggered — the last completed fork run is still
`32100752354` (created 2026-08-18T04:52:50Z), the same one my last review
analyzed job-by-job. So all of Section 1's code-level analysis carries forward
unchanged from that round; I'm not re-deriving it from scratch to avoid
manufacturing false novelty. What I did re-verify independently this round is
below, and one part of it is genuinely new information.
# 1. Code Change Review
## 1.1 Core Logic Analysis
Unchanged since 2026-08-19T04:26:46Z. Spot-checked rather than re-derived
from zero: `pom.xml:68-74,669` still carries
`maven.compiler.release=${java.version}` feeding `<release>` in
`maven-compiler-plugin`, and the Java-8-pinned module list
(`seatunnel-ci-tools`, `connector-cdc-tidb-e2e`, `connector-tdengine-e2e`,
`connector-typesense-e2e`, `connector-fluss-e2e`, `seatunnel-format-protobuf`,
and the four `seatunnel-flink-examples` modules) is still present and
unchanged. No file in this PR's diff has moved since the last round.
## 1.2 Compatibility Impact
Unchanged: **Incompatible.** Raising the minimum Java runtime from 8 to 11
breaks any Java-8 client, Zeta master/worker, or Flink/Spark cluster node
loading a SeaTunnel connector jar. Documented in
`docs/{en,zh}/introduction/concepts/incompatible-changes.md` with a migration
guide; carryover Issue 7 (the "every published jar is v55" doc claim being
inaccurate against the ten Java-8-pinned modules) is still open.
## 1.3 Performance / Side-Effect Analysis
No change since last round.
## 1.4 Error Handling and Logging — updated evidence on the one open blocker
**Issue 5 (High) — still open, and now with stronger and partially new
evidence.**
- **Location**: branch state (`compare/dev...b16d233d8469`); no new fork run
since `32100752354`.
- **What changed since my last review**: `compare/dev...b16d233d8469` now
reports `ahead_by=38, behind_by=42` (was `behind_by=38` at my last review 12
hours ago) — 4 more commits landed on `dev` with still no sync: `c7401bf14d7`,
`ce196d8faa0`, `b3420260d4e`, `5b358f8b4a9`. I checked each one; none touches
JDK/CI-matrix logic in a way that changes my prior conclusion, **with one
exception that is genuinely new and relevant**: `5b358f8b4a9` (`[Fix][CI] Add
fail-fast: false to backend.yml matrix jobs`, #11873) adds `fail-fast: false`
to the exact kind of matrix jobs that produced the 4 cancelled legs I
documented in the last review's fork run (`all-connectors-it-5 (17)`,
`all-connectors-it-3 (11)`, `kudu-connector-it (11)`, `mysql-cdc-connector-it
(11)`, all cancelled as fail-fast side effects of the 3 real failures). Syncing
`dev` now would not only pick up `#11231` (the Kafka exactly-once test fix I
asked for two rounds ago, still not present on this branch) but would also mean
a subsequent run doesn't lose those 4 legs to cascading cancellation — i.e.,
the confirming run I've been asking for would actually be more complete than
the one I already have, not just newer.
- **Potential risk**: unchanged from last round — none of the three prior
failures (Maven Central 429, `DatabendCDCSinkIT` timing,
`MysqlCDCWithFlinkSchemaChangeIT` timing) look PR-caused, but the bar of "one
clean, complete, synced run" still hasn't been met, and divergence is growing
rather than shrinking.
- **Best improvement**: unchanged — sync `dev`, push, let one full run
complete. The newly-landed `fail-fast: false` change is an added reason to do
this now rather than later, since it directly removes the cancellation noise
that made the last run partially inconclusive.
- **Severity**: High. **Raised by another reviewer**: No — this is the same
blocker from my 2026-08-16, 2026-08-18T03:41, and 2026-08-19T04:26 reviews,
still unresolved, now with one additional piece of supporting evidence for why
a sync is worth doing today rather than deferring further.
All other issues (7-10, 12, 13, 15-20, and the `spark-2.4.6` bake-target
finding) are unchanged from the 2026-08-19T04:26:46Z review — no file touching
them has moved.
# 2. Code Quality Assessment
## 2.1 Coding Standards
Unchanged — clean, unchanged since last round.
## 2.2 Test Coverage and Test Stability
No new CI run exists to re-assess. The last completed fork run
(`32100752354`) remains: 76 success / 3 failure / 4 cancelled / 8 skipped — not
a clean pass, and none of the 3 failures traced to this PR's own diff, as
detailed in the last review. No test in the diff has changed since then.
Stability rating for the PR's own test-adjacent changes
(workflow/docker/jvm-options files, no actual UT/E2E test code touched by this
PR): **Stable** — the PR itself adds no flaky test code; the open risk is
entirely about getting one uncancelled CI run on a synced head, not about test
code quality.
## 2.3 Documentation Updates
Unchanged. Carryover Issue 7 (doc overstates "every jar is v55" against 10
known Java-8-pinned modules) remains open.
# 3. Architectural Soundness
## 3.1 Elegance of the Solution
Unchanged: long-term solution. The `maven.compiler.release` fix landed in
the prior commit closed the last structural enforcement gap.
## 3.2 Maintainability
Unchanged from last assessment.
## 3.3 Extensibility
Unchanged from last assessment.
## 3.4 Historical-Version Compatibility
Unchanged: this is an intentional, disclosed, documented incompatible change
(Java 8 runtimes can no longer load connector jars built by this reactor). No
further action needed on the compatibility-documentation front beyond fixing
carryover Issue 7's inaccurate module count.
# 4. Issue Summary
| # | Issue | Status at this head (`b16d233d8469`) | Severity |
|---|---|---|---|
| 1 | `benchmarks.yml` JDK 8 lane | Fixed | was High |
| 2 | No `maven.compiler.release` | Fixed, verified | was High |
| 3 | ASF Spark E2E image on Java 8 | Fixed | was High |
| 4 | `EngineImageJdkUpgrader` doesn't verify JDK swap | Fixed | was High |
| 5 | CI red / branch diverged from `dev` | **Still open — worse**:
`behind_by` now 42 (was 38 last round, was 31 the round before). New supporting
fact this round: `dev` just gained `fail-fast: false` on the matrix jobs
(#11873) that produced this PR's 4 cancelled CI legs, so a sync now yields a
more complete confirming run, not just a newer one. | High |
| 6 | Docs said `11-jre`, ships `11-jdk` | Fixed | was Medium |
| 7 | "every jar is v55" doc claim vs. 10 Java-8-pinned modules | Still open
| Medium |
| 8 | log4j stdout filter misses `seatunnel-metadata-export.sh` / `.cmd`
scripts | Still open | Medium |
| 9 | Kudu/Paimon Kerberos shims diverge; no `InvocationTargetException`
unwrap | Still open | Medium |
| 10 | Shipped `jvm_options` has JDK-8-only GC flags | Still open | Medium |
| 11 | `AmazondynamodbIT` widened tolerance | Fixed | was Medium |
| 12 | `EngineImageJdkUpgrader` never restores original `USER` | Still open
| Medium |
| 13 | Unpinned/mutable base image tags | Still open | Medium |
| 14 | Title/type understated scope | Fixed | was Medium |
| NEW-1 | `spark-2.4.6` bake target inherits JDK-11 base, no version guard |
Still open (non-blocking, unused path) | Medium |
| 15-19 | `--add-exports` doc gap; `--line-buffered` GNU-only; dead version
guard; no enforcer plugin; undocumented Spark 2.4 deprecation | Still open |
Low |
| 20 | `upgrade_compatibility.yml` on JDK 17 against released 2.3.13,
unexercised by this PR's CI | Still open | Low |
**Raised by another reviewer**: No, for every item above. All review rounds
on this PR to date are mine; there are no inline review comments and no issue
comments from anyone but me.
# 5. Merge Recommendation
### Conclusion: Changes requested — do not merge at this head
**Blockers (must-fix)**
1. **Issue 5 (High)** — Sync `dev` into this branch (42 commits behind and
still growing), push, and let one complete, uncancelled CI run finish. This is
the third consecutive round asking for the same action; the only thing that
changed this round is that `dev` now also carries a `fail-fast: false` fix
(#11873) that removes the cancellation noise seen in the last fork run, so the
sync is strictly more informative to do now than it was last round.
**Everything else that was blocking is already closed** — all four prior
High-severity code issues (1-4) remain fixed and re-verified.
**Recommended fixes — non-blocking** (unchanged from the previous round;
none is new)
- Issue 7: correct the "every published jar is v55" doc claim against the 10
known Java-8-pinned modules.
- Issue 8: extend the log4j stdout filter to `seatunnel-metadata-export.sh`
and `.cmd` entry points, or explicitly scope Windows out.
- Issue 9: align Kudu/Paimon Kerberos catch clauses and
`resetDefaultRealm()` ordering; unwrap `InvocationTargetException`.
- Issue 10: replace commented JDK-8-only GC flags with `-Xlog:gc*`
equivalents.
- Issues 12-13: restore the original `USER` in the derived E2E image; pin
base image tags.
- NEW-1: guard/comment the unused `spark-2.4.6` bake target against the
shared JDK-11 base.
- Issues 15-20: document `--add-exports` for Flink/Spark submitters; guard
`--line-buffered`; remove the dead version guard; add a `requireJavaVersion`
enforcer; record an explicit Spark 2.4 decision; exercise
`upgrade_compatibility.yml` at a synced head.
**Overall assessment**
Nothing on the code side changed since my last review — I verified that
rather than assumed it. The single remaining blocker is unchanged in kind but
has grown in size (`behind_by` 38→42) and has a small new reason to act now:
the upstream `fail-fast: false` fix means the next sync-and-rerun will actually
give a clean, complete signal instead of a partially-cancelled one. Once a
synced run comes back clean, and modulo the non-blocking cleanup list above,
this should be mergeable.
*Reviewed statically at head `b16d233d8469` (unchanged since the
2026-08-19T04:26:46Z review); no code from this PR was executed locally. This
round's independent verification: confirmed no new commit and no new fork CI
run via `git log` and the Actions API, re-checked `compare/dev...b16d233d8469`
(`ahead_by=38, behind_by=42`), and inspected the 4 newly-landed `dev` commits
individually, one of which (#11873) is new, relevant evidence for the existing
sync recommendation.*
--
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]
