DanielLeens commented on PR #11545: URL: https://github.com/apache/seatunnel/pull/11545#issuecomment-5379364148
Hi all, thanks for the patience on this one — I know it's already been through a lot of rounds. Since I'm the author here, I owe everyone an independent, no-favors pass rather than a rubber stamp, so I re-verified the current head from scratch (patches, CI logs, doc content) rather than just restating my own prior conclusions. Sharing the full result below, including how it lines up with @SEZ9's read. # What Problem Does This PR Solve? - **User pain point**: CI validated SeaTunnel almost exclusively on JDK 8/11 toolchains, so JDK 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 that the code itself now needs on JDK 9+. - **Fix approach**: Move the CI matrix from JDK 8/11 to JDK 11/17 (~40 `backend.yml` matrix entries plus CodeQL, publish-docker, upgrade-compatibility, and `benchmarks.yml`), raise the Maven compiler baseline to Java 11 via `maven.compiler.release` (with ten modules pinned back to `release=8` where they must still emit Java 8 bytecode), fix the handful of JDK-17 incompatibilities that surfaced (internal `sun.security.krb5.Config` access, a stale `commons-lang3` NPE, log4j-api's stdout notice, an E2E thread-leak false positive), and carry the runtime `--add-opens`/`--add-exports` flags plus a JDK-11-based Docker image into the actual shipped product. - **One-sentence summary**: The JDK-11 baseline raise and CI migration are structurally sound and I could independently reproduce every piece of it against the current head, but that same head's completed CI run is red for a PR-caused reason — the new Docker base image drops `python3`, breaking every Python-connector E2E lane — that both I and @SEZ9 already agree is a hard blocker. # 1. Code Change Review ## 1.1 Core Logic Analysis I did not just re-read the prior rounds' conclusions; I pulled the current head's file-level patches directly from the PR files API and cross-checked the CI job logs myself. Everything below is something I looked at directly on this pass. **Confirmed independently: `seatunnel-dist/src/main/docker/Dockerfile` drops `python3`.** ``` -FROM seatunnelhub/openjdk:8u342 as builder +FROM eclipse-temurin:11-jdk as builder ... -FROM seatunnelhub/openjdk:8u342 +FROM eclipse-temurin:11-jdk ``` That's the entire diff to this file — no `apt-get install python3` or equivalent anywhere. I then pulled the raw logs for the fork CI run against this exact head (`https://github.com/DanielLeens/seatunnel/actions/runs/32320672650`, jobs `all-connectors-it-5` on both JDK 11 and JDK 17) and found the deterministic failure text myself: ``` java.io.IOException: python.executable does not resolve to an executable file: /usr/bin/python3 ``` This reproduces identically on both new JDK lanes, which confirms it's the base-image swap, not a JDK-11-vs-17 difference. @SEZ9 reached the same root cause independently in the issue-comment thread — I agree with that read and consider it re-verified from the log text itself, not just from the thread's summary of it. **Runtime path this PR actually changes** (build/CI/deploy plane; the job-execution data path itself — Source → Transform → Sink — is untouched): ```text GitHub Actions matrix (backend.yml, CodeQL, publish-docker, upgrade-compatibility, benchmarks.yml) -> JDK 11 / JDK 17 toolchains (was 8 / 11) -> mvnw compile with maven.compiler.release=11 (10 modules pinned to release=8) -> seatunnel-dist/src/main/docker/Dockerfile: eclipse-temurin:11-jdk (was seatunnelhub/openjdk:8u342) -> missing python3 <-- blocks connector-python-e2e in this exact image -> config/jvm_{master,worker,client}_options, jvm_options: carry --add-opens/--add-exports needed at runtime once KuduUtil/PaimonSecurityContext's reflective Kerberos-config reload actually executes on 11+ ``` **Spot-checks I ran on other pieces of the diff, independently of the prior rounds' claims:** - `pom.xml:65-70`: `maven.compiler.release=${java.version}` is introduced and the `maven-compiler-plugin` default configuration is switched from `<source>/<target>` to `<release>` — this is the right fix (constrains `javac`'s API surface to the target, not just bytecode level, so compiling on a newer JDK can't silently link against APIs the target runtime lacks). I counted the modules that override this back to `release=8` and got exactly ten: `seatunnel-ci-tools`, `connector-cdc-tidb-e2e`, `connector-fluss-e2e`, `connector-tdengine-e2e`, `connector-typesense-e2e`, `seatunnel-flink-examples` (parent + `flink-13`/`flink-15`/`flink-20` children), `seatunnel-format-protobuf`. - `seatunnel-connectors-v2/connector-kudu/.../KuduUtil.java` and `connector-paimon/.../PaimonSecurityContext.java`: both replace a direct `import sun.security.krb5.Config;` (which doesn't compile on 17 — the package isn't exported by `java.security.jgss`) with a reflective `Class.forName("sun.security.krb5.Config").getMethod("refresh").invoke(null)` call. I checked whether the runtime side of this was actually wired up, and it is: all four `config/jvm_*options` files add `--add-exports=java.security.jgss/sun.security.krb5=ALL-UNNAMED` in the same PR, and `codeql.yaml`/`publish-docker.yaml`/`upgrade_compatibility.yml` all add the same flag to their own `JAVA_TOOL_OPTIONS`. Without that flag the reflective call would throw `IllegalAccessException` at `invoke()` time even though it compiles fine — good that this was tracked through to the runtime side rather than left to fail silently in the field. - `seatunnel-core/seatunnel-starter/.../ServerExecuteCommand.java`: the old `isAllocatingThreadGetName()` JDK-8u102-era check is replaced with `isUnsupportedJavaVersion()`, which is just `!SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_11)`. Correct and much simpler than what it replaced. - `seatunnel-core/seatunnel-starter/src/main/bin/{seatunnel.sh,seatunnel-connector.sh}`: both scripts now grep out a log4j-api stdout notice that only appears on JDK 9+ (`sun.reflect.Reflection.getCallerClass is not supported`), and both correctly capture `PIPESTATUS[0]` under `set +e`/`set -e` to preserve the real Java exit code through the pipe instead of reporting grep's. This is a real, non-obvious correctness fix — without it, a failing `java` invocation piped into `grep -v` would previously have been reported as success once the notice line started appearing on 11/17 (since a `grep -v` that still matches at least one other line exits 0 regardless of what the upstream process did). I traced this carefully because a silent exit-code inversion in the CLI's own launch script would be a serious regression, and I'm satisfied it's implemented correctly. - `seatunnel-connectors-v2/connector-fluss/pom.xml`: pins and shades `commons-lang3` because `fluss-client` calls `org.apache.commons.lang3.SystemUtils.isJavaVersionAtLeast`, which NPEs on Java 11 against the old `commons-lang3` that Spark 2.4 puts on the classpath. This is a genuine, well-targeted compatibility fix (with a relocation so the connector's pinned copy can't be shadowed by the engine's own `commons-lang3`), not just JDK-matrix plumbing. - `seatunnel-e2e/.../SeaTunnelContainer.java`: the E2E thread-leak allowlist gains `s.startsWith("Cleaner-")` with a clear comment explaining it's `java.lang.ref.Cleaner`'s worker thread, a JDK 9+ replacement for the old finalizer-based native-resource cleanup used by some JDBC drivers, and that it lives for the JVM's lifetime by design. Broadening a leak-detection allowlist is always worth a second look since it can mask a real leak, but this one is narrowly scoped to an exact, well-known JDK thread name prefix and is well justified — I don't see risk of it swallowing an unrelated leak. - `seatunnel-e2e/.../EngineImageJdkUpgrader.java` (new file): derives Java-11 flavors of Flink/Spark E2E images that still ship a Java 8 JRE, by copying in a JDK and re-pointing the image's own `JAVA_HOME` symlink rather than switching to a different published tag (which would lose the shaded-Hadoop layout some of those images were adopted for in the first place). It fails the docker build loudly (`java -version | grep -q 'version "11'`) if the source image doesn't declare `JAVA_HOME` or if the swap didn't actually take effect, rather than silently continuing on Java 8. Good defensive design, and the Javadoc explains the "why" clearly enough that a future maintainer doesn't have to re-derive the reasoning. One smaller, self-contained behavioral change I want to flag transparently rather than wave through as "just JDK plumbing": `EdgeSocketPacketRecordDeserializer.decodeEncryption` now also catches `ProviderException` alongside `GeneralSecurityException` around the AES/GCM `Cipher` calls, and reports it through a new `PACKET_DECRYPT_ERROR` code instead of reusing `PACKET_DECODE_ERROR`. `EdgeSocketSourceReader.isDecryptionError` was updated to match. I checked the call site (`EdgeSocketSourceReader:200-213`): both branches are caught by the same outer `catch (EdgeSocketConnectorException)`, logged at WARN, and turned into a response code sent back to the collector (`DECRYPT_FAILED` vs `DECODE_FAILED`) — neither branch fails the job or changes control flow beyond that response code, so the blast radius of this change is small. It's a reasonable, narrowly-scoped hardening catch (some JCE crypto providers can throw the unchecked `ProviderException` for internal failures that `GeneralSe curityException` wouldn't catch), and separating it into its own error code is arguably a small improvement in diagnosability. Calling it out for visibility since it's actual behavior, not build tooling, even though I don't think it needs to block merge. ## 1.2 Compatibility Impact **Partially incompatible, and this is correctly documented.** Raising the minimum Java runtime from 8 to 11 is a breaking change for any deployment still running the SeaTunnel process itself (client, Zeta master/worker, or the Flink/Spark cluster SeaTunnel submits to) on Java 8. I read the actual doc content added at `docs/en/introduction/concepts/incompatible-changes.md` (and its `docs/zh` counterpart) rather than trusting that it exists, and it's genuinely thorough: it calls out the exact `UnsupportedClassVersionError` message users will see, states plainly that Flink and Spark clusters (not just the submitting client) must also run 11+, gives the specific Flink/Spark version floors (Flink 1.13+, Spark 3.0+ per SPARK-24417, with Spark 2.4 explicitly called "does start but not recommended" due to reflective-access warnings and a stale-`commons-lang3` classpath issue), and warns customized `jvm_options` users about JDK-11-removed flags like `-XX:+UseConcMarkSweepGC`/`-XX:MaxPermSi ze`. That's a more complete migration note than this class of breaking change usually gets. Compiled bytecode stays at Java 8 for the ten pinned modules, so downstream consumers of those specific artifacts on an 8 JVM are unaffected. No SeaTunnel config `Option`, public API, or job-file format changes. The Docker image swap (`8u342` → `11-jdk`) is a full JDK either way, so `jps`/`jstack`/`jmap` stay available as documented — but see Issue 1: the missing `python3` toolchain is a real compatibility regression against the prior image that isn't (and shouldn't be, until fixed) called out anywhere. ## 1.3 Performance / Side-Effect Analysis No hot-path/runtime performance impact — this is build and deployment tooling plus a handful of narrow, non-hot-path compatibility shims (reflective Kerberos config reload happens only on `krb5.conf` reload, not per-record). The `--add-opens`/`--add-exports` flags added to `config/jvm_*_options` are exactly the set CI now exercises via the matching `JAVA_TOOL_OPTIONS` additions, so there's no unvalidated gap between what CI runs and what ships — contingent on Issue 1 below actually landing so that claim is backed by a clean run rather than a red one. ## 1.4 Error Handling and Logging Reflective-access failures in `KuduUtil`/`PaimonSecurityContext` are caught narrowly (`ReflectiveOperationException`, plus `RuntimeException` in the Kudu variant to also catch `IllegalAccessException`/`InaccessibleObjectException` if the `--add-exports` flag is ever missing) and logged at WARN with the same message and fallback behavior as before — a missing flag degrades to "default realm still used" rather than crashing, matching pre-existing behavior. The shell-script `PIPESTATUS[0]` fix (see 1.1) is itself an error-handling correctness fix: without it, a failing `java` launch could be silently reported as a successful script exit once the new JDK-9+ stdout notice started appearing. The one place error handling is currently insufficient is the Docker image itself: a missing `python3` toolchain surfaces only as a runtime `IOException` when a Python connector job actually executes, not as a build-time or image-smoke-test failure — see Issue 1. --- **Issue 1 (High) — `seatunnel-dist/src/main/docker/Dockerfile`'s base-image swap drops `python3`, breaking the official Docker image for Python-connector users** - **Location**: `seatunnel-dist/src/main/docker/Dockerfile:1,15`. - **Problem**: Verified directly against the current head's patch (no `python3` install anywhere in the file or the rest of the diff) and against the raw logs of the completed fork CI run for this exact head, which fails deterministically on both `all-connectors-it-5 (11, ubuntu-latest)` and `all-connectors-it-5 (17, ubuntu-latest)` with `java.io.IOException: python.executable does not resolve to an executable file: /usr/bin/python3`. - **Potential risk**: This is the repository's official published Docker image. Any user relying on the Python connector against this image breaks on upgrade with an opaque runtime error surfacing only when the job actually executes, not at build or startup time. - **Best improvement**: Install `python3` (Option A: `apt-get install -y python3` in the final stage of the new `eclipse-temurin:11-jdk`-based Dockerfile; Option B: audit for anything else `seatunnelhub/openjdk:8u342` silently provided beyond `python3` that other E2E lanes might also depend on, and cover that in the same pass), then get a fresh, fully-completed CI run confirming the Python-connector lanes are green on both JDK 11 and JDK 17. - **Severity**: High — sole confirmed, reproducible, PR-caused CI failure at the current head. - **Raised by another reviewer**: Yes (@SEZ9), and independently re-confirmed by me directly against the log text on this pass. **Issue 2 (Low) — `CouchbaseIT` failure on `all-connectors-it-7 (17, ubuntu-latest)` is very likely a pre-existing, unrelated flake** - **Location**: `all-connectors-it-7 (17, ubuntu-latest)` lane; container bootstrap for `couchbase/server:community-7.1.1`. - **Problem**: I pulled the raw log for this job myself — it fails with `Could not start container` / `Could not perform request against couchbase HTTP endpoint`, i.e. a Testcontainers-level bootstrap failure, not a JDK-version-specific class-loading or reflection error. Neither I nor @SEZ9 could tie it to the JDK bump or the base-image change. This matches a known, already-tracked `ubuntu-latest` container-bootstrap race for this exact test (unrelated fix already proposed elsewhere for `startupAttempts`/log-consumer timing), so the evidence points to pre-existing environmental flake rather than something this PR introduced. - **Potential risk**: Low — if it does turn out to be JDK-related after a clean re-run, it would need separate investigation, but nothing in the log points that way. - **Best improvement**: Re-check the same lane once Issue 1's fix produces a clean run; if it clears, no further action needed here; if it reproduces, escalate separately rather than blocking this PR on it. - **Severity**: Low. - **Raised by another reviewer**: Yes (@SEZ9). **Issue 3 (Medium) — `upgrade_compatibility.yml` now runs on JDK 17 against the last released `2.3.13` artifacts, with no CI evidence yet that the upgrade path itself works cleanly on the new baseline** - **Location**: `.github/workflows/upgrade_compatibility.yml`. - **Problem**: The workflow was moved to the new JDK matrix along with everything else, and I confirmed the `--add-exports` flag was carried into its `JAVA_TOOL_OPTIONS` too — but this specific job's actual behavior under the new baseline (restoring/running against a real `2.3.13` release artifact) isn't independently exercised or confirmed by any evidence in this PR's own CI history that I could find. - **Potential risk**: If the upgrade-compatibility check has a latent JDK-17-specific issue, it would only surface after this PR merges and the workflow next runs against a real release artifact — outside the window where it's easy to attribute back to this change. - **Best improvement**: Get at least one completed, green run of this workflow against the new matrix (can be a manual `workflow_dispatch` trigger) before or shortly after merge, and note the result in the PR description. - **Severity**: Medium. - **Raised by another reviewer**: No. **Issue 4 (Medium) — No Maven Enforcer `requireJavaVersion` rule to give a contributor still on JDK 8 a clear, actionable error** - **Location**: root `pom.xml` — Maven Enforcer configuration (absent). - **Problem**: A contributor who still has only JDK 8 locally will now hit a raw `javac`/toolchain failure (or, worse, a confusing failure further into the build) rather than an explicit "this project requires JDK 11+" message at the very start of the build. - **Potential risk**: Low-friction but real first-touch confusion for existing contributors immediately after this baseline raise lands. - **Best improvement**: Add a `maven-enforcer-plugin` `requireJavaVersion` rule (`<version>[11,)</version>`) with a clear failure message, enforced in the root `pom.xml`'s enforcer execution. - **Severity**: Medium — non-blocking, pure quality-of-life for contributors, but cheap to add and worth doing before or shortly after merge. - **Raised by another reviewer**: No. **Issue 5 (Low) — A couple of small, previously-identified low-severity gaps remain open** - **Location**: various — `docs/en(zh)/introduction/concepts/incompatible-changes.md` (Flink/Spark cluster-side `--add-exports` for Kerberos-secured Kudu/Paimon jobs not mentioned); `seatunnel-core/seatunnel-starter/src/main/bin/{seatunnel.sh,seatunnel-connector.sh}` (`grep --line-buffered` is a GNU extension, used without a documented Linux-only assumption). - **Problem**: (a) The migration guide documents `config/jvm_*_options` flag changes for the SeaTunnel process itself, but doesn't mention that a Flink/Spark cluster running a Kerberos-secured Kudu or Paimon connector job on JDK 11+ will also need `--add-exports=java.security.jgss/sun.security.krb5=ALL-UNNAMED` passed via that engine's own JVM-options mechanism (e.g. Flink's `env.java.opts`, Spark's `extraJavaOptions`), since the reflective call executes inside the TaskManager/executor JVM that loads the connector jar, not just inside the SeaTunnel client. (b) `--line-buffered` isn't portable to non-GNU `grep`; fine for the documented Linux deployment target (Docker image, CI), but worth a one-line comment saying so if a contributor ever runs these scripts on a different platform. I confirmed while reviewing that the Spark 2.4-on-Java-11 deprecation point I'd have otherwise flagged here is **already** covered in the incompatible-changes doc, so that item can come off this list. - **Potential risk**: Low on both — narrow, discoverable-in-testing gaps rather than something that silently corrupts data or breaks a common path. - **Best improvement**: (a) add one sentence to the incompatible-changes doc's migration guide about the Flink/Spark cluster-side flag for Kerberos-secured Kudu/Paimon jobs; (b) a short comment noting the Linux/GNU-grep assumption in the two shell scripts. - **Severity**: Low — non-blocking, author's discretion on timing. - **Raised by another reviewer**: No. # 2. Code Quality Assessment ## 2.1 Coding Standards This is primarily CI/build-configuration and Dockerfile changes; the handful of actual Java-level fixes (`KuduUtil`/`PaimonSecurityContext` reflection, the `EdgeSocket` exception handling, `EngineImageJdkUpgrader`) are narrowly scoped, come with clear Javadoc/comments explaining the "why" (not just the "what" — e.g. the exact JPMS export gap that forces the reflective call, or the exact reason a full JDK is needed over a JRE), and are consistent with the project's existing patterns for workarounds of this kind. No new non-trivial method or field lacks an explanation for why it exists. ## 2.2 Test Coverage and Test Stability Not applicable in the "new business-logic UT/E2E" sense — this PR doesn't add new source-level features. It does touch existing E2E fixtures and test infrastructure, so I ran the mandated flaky-test check against those touch points specifically: - `AmazondynamodbIT.java:253` — `LocalDateTime.now()` → `LocalDateTime.now().truncatedTo(ChronoUnit.MICROS)`. This is a deterministic truncation applied before the value is used, not a timing-dependent wait; I confirmed the diff does **not** introduce a `Thread.sleep`/polling-based assertion here (an earlier round had flagged a widened 30s-polling version of this same test; the current head does not have that — it's the simple deterministic truncation). - `SeaTunnelContainer.java`'s thread-leak allowlist gains `"Cleaner-"` (discussed in 1.1) — this loosens a test assertion's matching criteria, but it does so with a specific, verifiable JDK-version-tied justification rather than a blanket widening, so I don't consider it a stability regression. - No new `Thread.sleep`-based waits, no nondeterministic assertions, and no shared mutable static state were introduced in the touched E2E infrastructure files that I reviewed (`ConnectorPackageServiceContainer`, `AbstractTestFlinkContainer`, `Flink13/14/20Container`, `Spark2/3Container`, `EngineImageJdkUpgrader`). **Stability rating: Risk present** — not because of any flaky-test pattern in the PR's own test code, but solely because Issue 1 is a real, CI-demonstrated, PR-caused E2E failure at the current head that must clear before this can be called `Stable`. ## 2.3 Documentation Updates `docs/{en,zh}/introduction/concepts/incompatible-changes.md`, `docker.md`, and `kubernetes.mdx` are all updated and I read them directly rather than assuming they matched the code — they're accurate and consistent with the `11-jdk` base image in both locales, and the incompatible-changes entry is unusually thorough (see 1.2). The missing-`python3` gap (Issue 1) isn't documented anywhere, which is the right call — it should be fixed, not documented as a known limitation, matching the project's own stated preference. # 3. Architectural Soundness ## 3.1 Elegance of the Solution — Precise fix Switching to `maven.compiler.release` instead of relying on `-source`/`-target` alone is the textbook-correct fix for the specific problem it targets, and pinning only the ten modules that must stay on Java 8 bytecode (rather than a blanket override, or leaving the whole build on 8) is a minimal-footprint, precisely-targeted change. The reflective Kerberos-config-reload workaround and the `EngineImageJdkUpgrader` derived-image approach are both narrowly scoped to the exact JPMS/JDK-version problem they solve, with the reasoning documented in place rather than left implicit. ## 3.2 Maintainability The CI matrix replacement (8/11 → 11/17, not additive) keeps the CI leg count unchanged while moving the comparison baseline forward — the right trade for keeping CI runtime bounded while still gaining JDK 17 signal going forward. ## 3.3 Extensibility `EngineImageJdkUpgrader`'s pattern (derive-in-place rather than switch published tags) generalizes cleanly to a future JDK bump on the same E2E images without having to re-solve the "which upstream tag has both the JDK version and the Hadoop/Spark layout we need" problem again. ## 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," which is correctly documented as an incompatible change (see 1.2). The one gap is that the Docker image's toolchain regression (Issue 1) is itself a compatibility break against the prior image that isn't yet named in that same doc — appropriately, since the plan is to fix it rather than document it as accepted. # 4. Issue Summary | # | Issue | Location | Severity | |---|-------|----------|----------| | 1 | Docker base-image swap drops `python3`; breaks Python-connector E2E on both new JDK lanes, independently reproduced from the current head's CI logs | seatunnel-dist/src/main/docker/Dockerfile:1,15 | High | | 2 | `CouchbaseIT` failure on the 17/ubuntu lane not tied to this PR; container-bootstrap error text points to pre-existing flake | all-connectors-it-7 (17, ubuntu-latest) | Low | | 3 | `upgrade_compatibility.yml`'s JDK-17 run against `2.3.13` has no confirmed-green run yet | .github/workflows/upgrade_compatibility.yml | Medium | | 4 | No Maven Enforcer `requireJavaVersion` rule for contributors still on JDK 8 | pom.xml | Medium | | 5 | Flink/Spark cluster-side `--add-exports` for Kerberos-secured Kudu/Paimon jobs undocumented; GNU-`grep`-only assumption in shell scripts undocumented | incompatible-changes.md; seatunnel.sh / seatunnel-connector.sh | Low | # 5. Merge Recommendation ### Conclusion: Ready to merge after fixes **1. Blockers — must be fixed** 1. **Issue 1 (High)** — Install `python3` in the new Dockerfile base image and get a fresh, fully-completed CI run confirming the Python-connector lanes pass on both JDK 11 and JDK 17. This is the sole confirmed, reproducible, PR-caused failure at the current head — @SEZ9 and I independently reached the same conclusion from the same CI run, and I re-verified it a third time directly against the raw job logs on this pass. **2. Recommended fixes — non-blocking** 1. Issue 2 — re-check `CouchbaseIT` on the same lane once the Issue 1 fix lands; if it clears, no further action; if not, track separately as an environmental issue. 2. Issue 3 — get at least one confirmed-green run of `upgrade_compatibility.yml` on the new matrix. 3. Issue 4 — add a Maven Enforcer `requireJavaVersion` rule. 4. Issue 5 — close out the two small documentation/portability nits at the author's discretion. **Overall assessment.** This PR does exactly what a JDK-baseline-raise PR should do: move CI forward first so toolchain incompatibilities are caught by automation rather than by users, fix each one narrowly with the reasoning documented in place, and then close the loop by carrying the same runtime flags and JDK baseline into the actually-shipped Docker image and JVM option files — the step that's easy to skip and that this PR explicitly doesn't skip. Having gone through the diff myself end to end rather than deferring to the prior rounds' conclusions, I didn't find anything that changes that read. The one place the same discipline currently falls short is the Docker image itself, where the base-image swap silently dropped a toolchain component (`python3`) that a real connector's E2E suite — and presumably real deployments — depend on. That's a CI-proven, not merely suspected, blocker. Once Issue 1 is fixed and a clean run confirms it, I don't see a remaining source-level ob jection to merging. As the PR author is also the account posting this review, this is not an independent approval, and a write-capable maintainer still needs to perform the final review and confirm the CI outcome once Issue 1's fix produces a green run. -- 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]
