[Feature][Spark] Support Spark 3.5.8 by goutamadwant · Pull Request #11307 · apache/seatunnel

goutamadwant · 2026-07-03T06:49:38Z

Purpose of this pull request

Add Spark 3.5.8 support to SeaTunnel through a separate starter, distribution artifact, Unix/Windows launchers and E2E container. The existing Spark 3.3 starter and default launch path remain available.

Reuse the existing Spark 3.3 translation layer, select the row-encoder API at runtime, and explicitly implement the batch/streaming commit-coordinator method.
Replace Unix eval with a NUL-delimited argument-file handoff and quoted array invocation. Preserve JVM exit status and clean up launcher-owned temporary files.
Correct Windows argument capture, classpath construction and JVM failure handling, with the remaining command-string limitations documented.
Verify the packaged starter's isolated classpath and single compatible SLF4J binding.
Adapt Hudi/Iceberg S3 test dependencies and bucket isolation, Druid decimal fixtures, and Kafka/Python test compatibility for the new runtime.
Add row-encoder, finite micro-batch sink-adapter and packaged-launcher coverage, and update English and Chinese engine, quick-start and command documentation.
Isolate the test fixtures: restore pre-existing starter properties exactly and prevent the micro-batch fixture from borrowing or stopping another test's Spark context.

Does this PR introduce any user-facing change?

Yes. Users can select seatunnel-spark-3.5-starter.jar and start-seatunnel-spark-3.5-connector-v2.sh / .cmd for Spark 3.5.8. Existing Spark 3.3 artifacts and defaults remain in place. Spark 3.4 guidance is unchanged; this PR does not add Spark 3.4 compatibility coverage.

There is no intentional breaking change to existing configuration defaults, public APIs or data/checkpoint formats. The test-isolation follow-up changes tests only.

Support and verification boundaries:

The public starter remains batch-only. The finite micro-batch test verifies runtime row encoding and the streaming sink adapter, not public-starter streaming, SeaTunnel streaming sources, checkpoint restore or exactly-once behavior.
Translation code is compiled against Spark 3.3. Selected runtime checks do not establish compatibility with every Catalyst/DataSource V2 API or connector.
The packaged Unix test invokes a real JVM and captures arguments at the spark-submit boundary; it is not a distributed Spark submission.
Windows still hands a command string to spark-submit.cmd. Arbitrary embedded quotes and shell metacharacters are unsupported; do not pass untrusted values through this launcher.
The Spark 3.5 connector container is scheduled-only (testInPR=false). Normal PR CI does not establish its connector/deployment matrix.

How was this patch tested?

Regression evidence

Both new regression tests failed before the fixture fixes:

Cleanup discarded a pre-existing empty starter property instead of restoring it. Coverage now checks absent, empty and nonempty values for all three starter properties.
The micro-batch fixture reused a pre-existing Spark context and stopped it. Coverage now verifies rejection before fixture side effects, context survival, and a successful action on that original context.

The property fixture uses JUnit's system-properties resource lock. The Spark fixture runs in isolation with same-thread execution, checks for an existing context before starting, and retains the original exact two-row/INSERT-kind commit assertions and query/session cleanup.

Local follow-up validation

The following results cover the test-isolation follow-up at 1822620f09823e2d7762334d260e110c2583f977; its committed source matches the locally validated candidate.

JDK 11 and 17: scoped Spark 3.5 reactor verification passed on each JDK; 24 selected unit tests passed per run. The packaged starter IT executed six cases successfully and skipped four Windows-only cases as expected on each JDK.
The real Spark 3.5.8 finite file stream terminated, reported exactly two input rows and committed the expected INSERT values on each JDK.
Adjacent Spark 3.3 reactor verification on JDK 11 passed all 12 selected tests, including the real batch sink/copy-safety regression.
The freshly compiled existing SparkSinkTest.testSparkSinkWriteDataWithCopy also passed on the Spark 3.5.8 classpath under JDK 11 and 17, using an isolated process and temporary warehouse. This is a local adapter-level batch check, not a distributed public-launcher job or a connector-matrix result.
With JUnit method and class parallel execution enabled, all 24 selected tests passed on JDK 11. The Spark fixture remains isolated and the property fixture uses the shared resource lock.
Scoped spotless:apply and whitespace checks passed. Do not interpret the hosted baseline below as validation of this follow-up.

Run the following with each selected JDK:

./mvnw -B -T 1 \
  -pl seatunnel-core/seatunnel-spark-starter/seatunnel-spark-3.5-starter -am \
  -Dtest=SparkStarterTest,Spark35LauncherTest,SparkRowEncoderTest,Spark35RowEncoderTest,Spark35BatchWriteTest \
  -Dsurefire.failIfNoSpecifiedTests=false \
  -Dit.test=TestSpark35StarterJarContentIT -Dit.failIfNoSpecifiedTests=false \
  -DskipIT=false -Dskip.ui=true verify

Adjacent Spark 3.3 verification:

./mvnw -B -T 1 \
  -pl seatunnel-core/seatunnel-spark-starter/seatunnel-spark-3-starter -am \
  -Dtest=SparkStarterTest,SparkRowEncoderTest,SparkSinkTest \
  -Dsurefire.failIfNoSpecifiedTests=false \
  -DskipIT=true -Dskip.ui=true verify

The parallel-execution check used the Spark 3.5 command with -DskipIT=true and these additional properties: -Djunit.jupiter.execution.parallel.enabled=true -Djunit.jupiter.execution.parallel.mode.default=concurrent -Djunit.jupiter.execution.parallel.mode.classes.default=concurrent.

Hosted evidence and remaining limits

The baseline Build passed at 47ae1be3d4997bd63ad9daffa7f7a8032c5df03a. Its native Windows JDK 11 and JDK 17 logs each show ten packaged-starter cases: nine executed successfully, one intentional Unix-only skip, and no failures or errors. This replaces the obsolete queued-run and unverified-Windows statements in the previous description.

That successful hosted run predates the test-isolation follow-up. Fresh Build run 41580340332 was triggered by the push of 1822620f09823e2d7762334d260e110c2583f977 and is queued at this update; no passing hosted result is claimed for the new revision. The Windows launcher checks do not submit an actual Windows Spark job.

Still not established: the selected/scheduled Spark 3.5 connector matrix, including applicable Hudi/Iceberg S3 paths and the intended distributed deployment mode; native Windows Spark-job execution; public-starter streaming or recovery. Local runtime tests and green aggregate CI do not substitute for these checks.

Check list

If any new Jar binary package adding in your PR, please add License Notice according New License Guide
If necessary, please update the documentation to describe the new feature. https://github.com/apache/seatunnel/tree/dev/docs
If necessary, please update incompatible-changes.md to describe the incompatibility caused by this PR.
If you are contributing the connector code, please check that the following files are updated:
1. Update plugin-mapping.properties and add new connector information in it
2. Update the pom file of seatunnel-dist
3. Add ci label in label-scope-conf
4. Add e2e testcase in seatunnel-e2e
5. Update connector plugin_config

DanielLeens

Thanks for working on this. I reviewed the current head against the touched Spark starter / translation path instead of only looking at the version bumps.

What this PR changes

User pain: Spark 3.3-era APIs and dependency coordinates no longer line up with a Spark 3.5.8 runtime.
Fix approach: bump the Spark dependency set to 3.5.8, adapt the row encoder call site, and opt into Spark's commit coordinator on the batch sink path.
One-line summary: the API-direction changes look reasonable, but the current head is still blocked by a real merge-conflict state against dev.

Runtime chain I checked

Spark starter transform path
  -> TransformExecuteProcessor.sparkTransform(...)
      -> Encoders.row(outputManager.getTableSchema())

Spark sink commit path
  -> SeaTunnelBatchWrite.useCommitCoordinator() == true
      -> driver-side aggregated commit remains serialized through Spark's commit coordinator

Current-head findings

The Spark API adaptation in seatunnel-core/seatunnel-spark-starter/seatunnel-spark-starter-common/src/main/java/org/apache/seatunnel/core/starter/spark/execution/TransformExecuteProcessor.java:172-184 is directionally correct for Spark 3.5, because it replaces the removed RowEncoder.apply(...) usage with Encoders.row(...) on the actual transform output path.
The sink-side change in seatunnel-translation/seatunnel-translation-spark/seatunnel-translation-spark-3.3/src/main/java/org/apache/seatunnel/translation/spark/sink/SeaTunnelBatchWrite.java:101-104 is also on the real commit path and keeps aggregated commits under Spark's coordinator.
I do not see a new flaky-test pattern in the tiny SparkSinkTest follow-up on this head.

Problem 1: GitHub is still reporting merge conflicts against dev

Location: PR merge state (mergeStateStatus=DIRTY, mergeable_state=dirty)
Why this is a blocker:
The current branch cannot be merged or meaningfully revalidated until the conflict against the latest dev is resolved. This PR also touches the shared Spark starter / translation dependency surface, so rebasing is not a cosmetic step here.
Risk:
Leaving the conflict unresolved means we cannot trust the final integrated dependency graph or the final CI result on the real merge candidate.
Best fix:
Please sync with the latest dev, resolve the conflicts, and rerun Build on the rebased head. Once that is done, I am happy to re-review the integrated result.
Severity: High
Already raised by others: No

Merge conclusion

Conclusion: can merge after fixes

Blocking items

Problem 1: resolve the current merge conflicts against dev and rerun CI on the rebased head.

Suggested follow-up

None beyond the conflict resolution on the current revision.

The overall direction makes sense for Spark 3.5.8 support. Once the branch is conflict-free, I can take another look at the final integrated head.

goutamadwant · 2026-07-03T14:13:06Z

Thanks @DanielLeens , rebased this on latest dev and resolved the root pom conflict.

I kept the Spark 3.5.8 update from this PR and took the current dev side for Jackson 2.15.4, maven-shade-plugin, and jackson-module-scala.

Also reran the focused Spark modules locally, plus the json serialization test that failed in the previous CI run. Both pass now. CI is running again on the updated branch.

DanielLeens

Thanks for the update. I re-reviewed the latest head from the real Spark starter / translation / E2E runtime surface again, not only the version bumps.

What this PR solves

User pain: Spark 3.3-era APIs and dependency coordinates no longer line up with a Spark 3.5.8 runtime.
Fix approach: bump the Spark dependency set to 3.5.8, replace the removed row-encoder API usage, and keep the sink commit path aligned with the newer Spark surface.
One-line summary: the API-direction changes are reasonable, but the project's real Spark integration/runtime harness is still pinned to Spark 3.3.0, so the current head does not yet prove end-to-end Spark 3.5.8 support.

Runtime chain I checked

Spark starter transform path
  -> pom.xml switches the shared Spark dependency property to 3.5.8
  -> TransformExecuteProcessor.sparkTransform(...)
      -> Encoders.row(outputManager.getTableSchema()) on the transform output path

Spark integration / validation path
  -> seatunnel-spark-starter-common and translation-spark compile against 3.5.8
  -> seatunnel-e2e Spark container still boots tyrantlucifer/spark:3.3.0
  -> the repository's real Spark E2E/runtime surface is therefore still a 3.3.0 environment

Findings

Problem 1: the real Spark integration harness is still pinned to 3.3.0 while the PR claims 3.5.8 support

Location:
- pom.xml:81-82
- seatunnel-core/seatunnel-spark-starter/seatunnel-spark-starter-common/src/main/java/org/apache/seatunnel/core/starter/spark/execution/TransformExecuteProcessor.java:179-183
- seatunnel-e2e/seatunnel-e2e-common/src/test/java/org/apache/seatunnel/e2e/common/container/spark/Spark3Container.java:47-49
Why this blocks:
the current head changes the shared Spark dependency surface to 3.5.8 and adapts the transform path to Encoders.row(...), but the repository's actual Spark integration container still starts tyrantlucifer/spark:3.3.0. That means the end-to-end validation path is still exercising a different runtime from the one this PR claims to support.
Risk:
a green 3.3-based integration path would still not prove 3.5.8 support, and the current red Build on this head is also hard to interpret while compile/runtime/test surfaces are split across two Spark versions.
Best fix:
- Option A (preferred): update the Spark E2E/runtime harness to a real 3.5.8 environment and rerun CI on that path.
- Option B: if the project wants to keep both 3.3 and 3.5 support, add an explicit 3.5 validation lane instead of replacing the dependency surface without updating the integration/runtime contract.
Severity: High
Already raised by others: No

Merge conclusion

Conclusion: can merge after fixes

Blocking items

Problem 1: align the real Spark integration/runtime validation surface with Spark 3.5.8 before claiming support for that version.

Suggested follow-up

After the runtime harness is aligned, rerun the full Spark-related CI surface on the updated head so the final signal comes from the same version family the PR is enabling.

Happy to take another pass once the Spark runtime/test surface is brought into the same 3.5.8 contract as the code changes.

Signed-off-by: goutamadwant <workwithgoutam@gmail.com>

DanielLeens

Thanks for the update. I re-reviewed the latest head from scratch.

The concrete runtime mismatch I called out last time is closed for me now: the Spark 3 path is no longer only updating the translation/starter dependencies on paper, it also moves the E2E/runtime harness onto the same 3.5.8 baseline through Spark3Container, ContainerUtil, and the Lance exclusion adjustment. On the source side, that makes the current head internally consistent.

At this point the remaining gate is CI rather than a new code-side blocker from my side. The current Build run is still red across several broad matrix lanes (all-connectors-it-*, transform-v2-it-*, engine-v2-it, jdbc-connectors-it-part-1, kafka-connector-it, connector-redis-it), and this branch is also slightly behind the latest dev (ahead_by=2, behind_by=3, compare status: diverged). Given that the Spark-specific runtime blocker is already addressed and the failing lanes now span suites outside the path I just rechecked, the lowest-cost next step is to sync the latest dev and rerun CI first. If the updated head is still red after that, please drop the new failing check link here and I can help narrow it down further.

goutamadwant · 2026-07-04T16:21:48Z

Thanks @DanielLeens . I merged latest dev into GH-11128 and pushed it.

The branch should be current with dev now, so I will wait for the new Build run to finish. If it is still red after this run, I will share the fresh failing check link here and narrow it from that result.

DanielLeens

Thanks for the update. I re-reviewed the latest head from scratch again against the current Spark runtime path, not the older version I blocked before.

Important update from the current head: the Spark 3 E2E runtime has now been moved together with the code path, so my earlier Spark-3.3-specific blocker no longer applies.

Runtime chain I checked

Spark batch transform path
  -> TransformExecuteProcessor.sparkTransform(...) [TransformExecuteProcessor.java:172-184]
      -> Encoders.row(outputManager.getTableSchema()) [L180]

batch sink commit path
  -> SeaTunnelBatchWrite.useCommitCoordinator() [SeaTunnelBatchWrite.java:101-104]
      -> commit coordinator enabled

current Spark 3 E2E runtime
  -> Spark3Container.getDockerImage() [Spark3Container.java:49-52]
      -> apache/spark:3.5.8

What I found on the current version

The current PR is no longer in the old “dependency bumped but runtime still on Spark 3.3” state.
The new encoder path does hit the real transform execution path.
The commit-coordinator change does hit the real batch sink commit path.
I do not see a new source-level blocker on the latest head.

CI note

The latest Build is still red, so I checked the failing job log.
In all-connectors-it-1 (11), the failing module is connector-http-e2e, specifically HttpIT.testStreamingSourceToPostgresqlSink, which times out in a streaming assertion.
The Spark translation / starter modules themselves build successfully in that run, and the current E2E runtime is already on Spark 3.5.8. Based on that evidence, I do not see the current red CI as a blocker caused by this PR's touched path.

Merge conclusion: can merge

Blocking items

None from my side on the current head.

Non-blocking suggestions

I would still keep an eye on the current red connector-http-e2e timeout and rerun / rebase as needed before merge, but I do not see a source-level reason to block this Spark 3.5.8 upgrade on that failure alone.

Overall, this looks good to me now.

DanielLeens · 2026-07-05T11:07:42Z

Rechecked the current head again.

I still do not see a new Spark code-side blocker from Daniel's side, but the branch is now further behind the latest dev (ahead_by=3, behind_by=12, compare status: diverged), and the current Build failure is spread across broad matrix lanes such as transform-v2-it-*, engine-v2-it, all-connectors-it-*, jdbc-connectors-it-part-1, kafka-connector-it, and connector-redis-it:
https://github.com/goutamadwant/seatunnel/actions/runs/28711971205

Those lanes are broader than the Spark 3.5.8 path I just rechecked, so the lowest-cost next step is to sync the latest dev and rerun CI first. If the rebased head is still red, please paste the new failing check link here and I can help narrow it down further.

DanielLeens

Thanks for the update. I re-reviewed the latest diff from the Spark dependency baseline through the Spark starter, translation writer, and E2E container path.

What this PR changes

Moves the Spark 3 baseline from Spark 3.3.0 to Spark 3.5.8.
Updates Spark starter / translation dependencies and excludes conflicting logging bridges.
Replaces the internal RowEncoder.apply(...) usage with Encoders.row(...) for Spark 3.5.
Adds useCommitCoordinator() to the Spark batch write path.
Updates the Spark 3 E2E container to apache/spark:3.5.8 and syncs docs.

Runtime path checked

User runs with Spark 3 engine
  -> seatunnel-spark-3-starter resolves Spark 3.5.8 dependencies
      -> transform execution
          -> TransformExecuteProcessor.sparkTransform(...)
              -> Encoders.row(outputSchema)
              -> Dataset.map(..., encoder)
      -> sink execution
          -> SeaTunnelBatchWrite.createBatchWriterFactory(...)
          -> useCommitCoordinator() = true
          -> Spark 3.5 commit coordinator handles writer commits

E2E path
  -> ContainerUtil.discoverTestContainers()
      -> SPARK_3_5
      -> Spark3Container uses apache/spark:3.5.8
      -> starts spark-class org.apache.spark.deploy.master.Master --host 0.0.0.0

Compatibility / side effects

SeaTunnel public API/config/checkpoint format: no change.
Spark 3 runtime dependency baseline changes from 3.3.0 to 3.5.8, which is the intended feature of this PR.
The module artifact path still contains spark-3.3, but the displayed module name and dependency baseline are adjusted toward Spark 3 generally. I do not see that as a runtime blocker in this PR.
The Spark 3.5.8 E2E container path may need CI attention if the new image has startup/ready timing differences, but the code direction is reasonable.

Tests

This PR changes Spark E2E container wiring and Spark sink test expectations. I did not run local tests in this environment; this review is based on the downloaded PR diff and source-level call-chain analysis.

Merge conclusion: can merge

Blocking items

None from my side.

Suggested follow-up

Keep an eye on the Spark 3.5.8 E2E container stability in CI; if image startup/ready behavior flakes, that should be handled in the E2E infra layer.

Overall, this looks like the required Spark 3.5.8 baseline upgrade with the expected Spark API adaptations, and I do not see a code-side blocker on the current head.

DanielLeens · 2026-07-07T01:22:07Z

Thanks for the update. I checked the current head against the latest dev again.

base branch: dev
head SHA: 115ecdc5419b1cfaefe8e237d4949495b51ee03d
compare status: diverged
ahead_by: 4
behind_by: 8
current failing signal: Build

On this round, the visible failing CI signal still does not look directly tied to the current PR-owned diff from Daniel's side, and the branch is still behind upstream. The lowest-cost next step is to sync with the latest dev and rerun CI first. If the same lane still fails on the updated head afterwards, please drop the new failing check link here and I can help narrow it down against the rebased revision.

DanielLeens

Thanks for the update. I re-reviewed the latest head from the Spark dependency baseline through the Spark starter, translation writer, and E2E container path.

What this PR solves

User pain: the Spark 3 path was still tied to Spark 3.3.x, while users need a current Spark 3.5.x runtime.
Fix approach: the PR moves the Spark 3 dependency baseline to Spark 3.5.8, updates the Spark 3 E2E image, and replaces the internal RowEncoder usage with Spark 3.5's public Encoders.row(...) path.
One-line summary: SeaTunnel's Spark 3 starter/test surface now targets Spark 3.5.8.

Runtime path checked

Spark job startup
  -> seatunnel-spark-3-starter dependencies use spark.3.5.8.version
  -> Spark3Container uses apache/spark:3.5.8 for E2E
  -> AbstractTestSparkContainer starts Spark master with explicit spark-class command
  -> TransformExecuteProcessor.sparkTransform(...)
      -> Encoders.row(output schema)
      -> Dataset.flatMap(transform function, encoder)

Merge conclusion

Conclusion: can merge

Blocking items

None from Daniel's source review.

Suggested follow-up

CI is still the remaining gate. Please let the current Build run finish; if it fails, we should inspect the concrete failing lane before asking for more source changes.

Compatibility is intentionally user-visible for the Spark 3 baseline, but it is documented in both English and Chinese docs. I do not see a new code-side blocker. LGTM from Daniel's side, pending CI.

DanielLeens

Thanks for the update. I re-reviewed the latest head for this PR after the new commit.

What this PR fixes

PR: [Feature][Spark] Support Spark 3.5.8
User-facing value: keeps the relevant SeaTunnel behavior/docs/tests aligned with the intended change.
In one sentence: I do not see a reopened source-level blocker on the current head.

Runtime / review path checked

Spark 3.5.8 support
  -> Spark launcher / translation module resolves Spark 3.5.8 artifacts
  -> docs and E2E config point to the supported runtime
  -> existing Spark execution path remains unchanged apart from version support

Current review findings

Changed files in the current full diff: 18.
Source-level conclusion: no new blocker from Daniel's side on the latest head.
Compatibility: I did not find an API/config/default/protocol/serialization break in the reviewed path.
Test stability: I did not see a new flaky-test pattern in the files touched by this PR during this static review.
Base sync fact recorded: compare status ahead, behind_by=0. This is recorded as queue metadata; I am not turning it into a standalone sync request unless it is needed to interpret a failing CI signal.

Testing and local verification

Fetched the PR head into a local review branch named seatunnel-review-11307.
Checked the current full diff, changed-file list, and latest commit list against dev.
I did not run local tests or a local build; this round is based on static diff and call-path review.

Merge conclusion

Conclusion: can merge after CI

Blocking items: none from Daniel's current source review.
Non-blocking suggestions: none from this round.

Current CI is PENDING; I am not approving until the required checks finish green.

DanielLeens

Rechecked the same head after the CI fact changed. From Daniel's side I still do not see a reopened source-level blocker on this revision; the current remaining gate is the Build workflow run (Label-when-reviewed) here: https://github.com/apache/seatunnel/runs/85805869855

This branch is currently behind the latest dev (compare=diverged, behind_by=7), so the lowest-cost next step is to sync with the latest dev and rerun Build first. If the same lane still fails after the sync, please paste the updated run link and I can help narrow it down against the refreshed head.

Note that any earlier Daniel review summary may still show historical state in GitHub's sidebar because I only have read-level review controls on this repository; this follow-up comment is the current Daniel-side technical conclusion for the same head.

SEZ9

Thanks for working on this. I re-reviewed the latest head 3780c41b3ca2 from scratch.

What this PR fixes

User pain: The Spark 3 starter and translation layer are pinned to the aging Spark 3.3.0 line, so users cannot run SeaTunnel jobs against current Spark 3.5.x clusters (#11128), and the E2E lane validates an image (tyrantlucifer/spark:3.3.0) nobody deploys anymore.
Fix approach: Bump the spark.3.3.0.version property to spark.3.5.8.version across the starter/translation/E2E poms, replace the removed RowEncoder.apply(...) with Encoders.row(...) in TransformExecuteProcessor, exclude Spark 3.5's log4j-slf4j2-impl/jul-to-slf4j bridges, keep explicit commit-coordinator behavior in SeaTunnelBatchWrite, and switch the E2E container to apache/spark:3.5.8 with an explicit master start command via the new getDockerCommand() hook.
One-line summary: The upgrade itself is mechanically sound and compiles/tests green against 3.5.8, but as written it silently drops runtime compatibility with Spark 3.3/3.4 (Encoders.row is 3.5+ only), the renamed root pom property risks breaking modules still referencing spark.3.3.0.version, and the non-root apache/spark:3.5.8 image plus a null-unsafe getDockerCommand() need hardening before merge.

Runtime chain I rechecked

SparkStarter.main() -> SparkExecution.execute()
  -> TransformExecuteProcessor.execute()  TransformExecuteProcessor.java:~100
    -> TransformExecuteProcessor.sparkTransform(transform, tableInfo)  TransformExecuteProcessor.java:170
      -> Encoders.row(outputManager.getTableSchema())  TransformExecuteProcessor.java:180   // replaces RowEncoder.apply(...); Encoders.row only exists in Spark >= 3.5, NoSuchMethodError on 3.3/3.4 runtimes
      -> Dataset.flatMap(new TransformMapPartitionsFunction(...), encoder)  TransformExecuteProcessor.java:181

E2E path:
Spark3Container.identifier() -> TestContainerId.SPARK_3_5  Spark3Container.java:41 / TestContainerId.java:37
  -> ContainerUtil.discoverTestContainers() filter on SPARK_3_5  ContainerUtil.java:388
  -> AbstractTestSparkContainer.startUp()  AbstractTestSparkContainer.java:76
    -> getDockerCommand()  AbstractTestSparkContainer.java:76-79   // NPE if a subclass returns null; only Spark3Container overrides it
      -> Spark3Container.getDockerCommand() -> spark-class Master --host 0.0.0.0  Spark3Container.java:56-62
    -> Spark3Container.getDockerImage() -> "apache/spark:3.5.8"  Spark3Container.java:51   // image runs as uid 185, affects copySeaTunnelStarterToContainer(master)  AbstractTestSparkContainer.java:80

Findings

Issue 1: Encoders.row() only exists in Spark 3.5+, so the Spark 3 starter now hard-fails at runtime on Spark 3.3/3.4 clusters with NoSuchMethodError

Location: seatunnel-core/seatunnel-spark-starter/seatunnel-spark-starter-common/src/main/java/org/apache/seatunnel/core/starter/spark/execution/TransformExecuteProcessor.java:180
Why it matters: - Root cause: Encoders.row(StructType) was introduced in Spark 3.5.0 as the replacement for the removed RowEncoder.apply(...). Since all Spark dependencies in this module use <scope>${spark.scope}</scope> (provided), the actual Spark classes come from the user's cluster at runtime.
Impact: any user who deploys the new seatunnel-spark-3-starter on an existing Spark 3.3.x or 3.4.x cluster will get a java.lang.NoSuchMethodError: org.apache.spark.sql.Encoders.row (or NoSuchMethodException) the moment a transform is executed — there is no graceful error message, no fallback, and the failure only surfaces mid-job. The docs still brand this as "the Spark 3 starter", which implies broader 3.x support.
Fix: either
(a) keep a reflection-based shim that tries RowEncoder.apply first and falls back to Encoders.row so 3.3/3.4 clusters keep working, or
(b) explicitly document this as a breaking change in docs/en/other-engine/incompatible-changes.md (the PR checklist item is unchecked) and fail fast with a clear "Spark >= 3.5 required" error during environment setup instead of a raw NoSuchMethodError.
Evidence: seatunnel-core/seatunnel-spark-starter/seatunnel-spark-starter-common/src/main/java/org/apache/seatunnel/core/starter/spark/execution/TransformExecuteProcessor.java:45 import org.apache.spark.sql.Encoders;
Suggested fix: Encoders.row(StructType) only exists since Spark 3.5.0. Because Spark is provided scope, users running this starter on a Spark 3.3/3.4 cluster will hit a raw NoSuchMethodError at runtime when the first transform runs. Please either add a reflection fallback to RowEncoder.apply(...) for older 3.x clusters, or document this as an incompatible change in incompatible-changes.md and fail fast with a clear "Spark >= 3.5 is required" message during environment initialization.
Severity: High

Issue 2: apache/spark:3.5.8 runs as non-root user 'spark' (uid 185), which can break file copy/exec of the SeaTunnel starter inside the E2E container

Location: seatunnel-e2e/seatunnel-e2e-common/src/test/java/org/apache/seatunnel/e2e/common/container/spark/Spark3Container.java:51
Why it matters: - Root cause: the old tyrantlucifer/spark:3.3.0 image ran as root, while the official apache/spark:3.5.8 image runs as the unprivileged spark user (uid 185). startUp() copies the SeaTunnel starter and logging config into the container (copySeaTunnelStarterToContainer(master)), and subsequent executeJob calls execute shell commands inside the container — both are sensitive to file ownership/permissions and to the working directory being writable.
Impact: E2E jobs may fail with Permission denied when writing checkpoint/log/tmp files under the SeaTunnel home, or when connectors write local state, producing flaky or hard-to-diagnose CI failures rather than clean assertion failures.
Fix: pin the container user explicitly (e.g., master.withCreateContainerCmdModifier(cmd -> cmd.withUser("root")) in Spark3Container) or ensure the SeaTunnel home and config paths are copied with permissions readable/writable by uid 185, and verify at least one full Spark 3.5 E2E suite (not just test-compile) actually runs green.
Evidence: seatunnel-e2e/seatunnel-e2e-common/src/test/java/org/apache/seatunnel/e2e/common/container/spark/Spark3Container.java:56 "/opt/spark/bin/spark-class",
Suggested fix: The official apache/spark:3.5.8 image runs as the non-root spark user (uid 185), unlike the old tyrantlucifer/spark:3.3.0 image. Files copied into the container by copySeaTunnelStarterToContainer and job-time writes (logs, checkpoints, tmp dirs) may hit Permission denied. Please either force root via withCreateContainerCmdModifier(cmd -> cmd.withUser("root")) or make sure the SeaTunnel home paths are writable by uid 185, and confirm a full Spark 3.5 E2E run passes — the PR description only shows test-compile for the E2E modules.
Severity: Medium

Issue 3: Renaming the root pom property spark.3.3.0.version -> spark.3.5.8.version will break the build of any module still referencing the old property, and bakes the patch version into the property name

Location: pom.xml:82
Why it matters: - Root cause: the version property in the root pom.xml was renamed rather than updated in place, so every ${spark.3.3.0.version} reference elsewhere in the repo (connectors, examples, seatunnel-dist, e2e poms) that was not updated in this PR will now fail dependency resolution with an unresolvable placeholder.
Impact: a full reactor build (./mvnw install from the root) can fail even though the modules the author tested individually pass, because the tests listed in the PR description only cover 6 modules. This is exactly the kind of happy-path-only validation that hides breakage until CI or a release build.
Fix: grep the whole repo for spark.3.3.0.version and update all remaining references in the same PR, and consider using a version-agnostic property name such as <spark.3.version> so future Spark upgrades do not require another repo-wide rename.
Suggested fix: Renaming the property from spark.3.3.0.version to spark.3.5.8.version will break any module that still references ${spark.3.3.0.version} (connectors, seatunnel-dist, examples). Please run a repo-wide grep for the old property name and update all references in this PR. Also consider a version-agnostic name like <spark.3.version> so the next Spark bump doesn't require another rename across the tree.
Severity: Medium

Issue 4: Module artifactId still says 'spark-3.3' while the display name and dependencies now target Spark 3.5.8, which is misleading for users and tooling

Location: seatunnel-translation/seatunnel-translation-spark/seatunnel-translation-spark-3.3/pom.xml:26
Why it matters: - Root cause: only the <name> element was changed to "SeaTunnel : Translation : Spark : 3" while the artifactId remains seatunnel-translation-spark-3.3 (and the directory is still seatunnel-translation-spark-3.3), even though the module now compiles against Spark 3.5.8 and (via Encoders.row) requires Spark 3.5+ at runtime.
Impact: users picking jars from the dist, and any assembly/plugin-discovery logic matching on the artifact name, will see a "3.3" artifact that actually does not work on Spark 3.3, which invites deployment mistakes and support tickets.
Fix: rename the module/artifactId (e.g., seatunnel-translation-spark-3) with the matching directory rename, or at minimum call out the naming mismatch and the effective minimum Spark version in the docs and incompatible-changes.md.
Suggested fix: The display name is now "Spark : 3" but the artifactId/directory is still seatunnel-translation-spark-3.3, while the module actually requires Spark 3.5+ at runtime after this PR. This mismatch will mislead users picking jars from the dist. Please rename the module to something like seatunnel-translation-spark-3 (with directory rename), or clearly document the effective minimum Spark version.
Severity: Low

Issue 5: AbstractTestSparkContainer.startUp() will NPE if a subclass returns null from getDockerCommand()

Location: seatunnel-e2e/seatunnel-e2e-common/src/test/java/org/apache/seatunnel/e2e/common/container/spark/AbstractTestSparkContainer.java:77
Why it matters: - Root cause: the new hook getDockerCommand() is a protected extension point, and startUp() immediately calls dockerCommand.isEmpty() on its result without a null guard. The base default returns Collections.emptyList(), but nothing stops a future subclass from returning null.
Impact: a null return produces an NPE during container startup, before any container logs exist, which is confusing to debug in E2E runs.
Fix: guard with if (dockerCommand != null && !dockerCommand.isEmpty()), or use CollectionUtils.isNotEmpty(dockerCommand), and document in the javadoc of getDockerCommand() that implementations must return a non-null list.
Evidence: seatunnel-e2e/seatunnel-e2e-common/src/test/java/org/apache/seatunnel/e2e/common/container/spark/AbstractTestSparkContainer.java:77 if (!dockerCommand.isEmpty()) {
Suggested fix: getDockerCommand() is an overridable hook — if a subclass ever returns null this will NPE during startup before any useful logs exist. Please use if (dockerCommand != null && !dockerCommand.isEmpty()) (or CollectionUtils.isNotEmpty) and note the non-null contract in the method's javadoc.
Severity: Low

Review conclusion

Conclusion: can merge after the blocking items are fixed

1. Blocking items

Issue 1: Encoders.row() only exists in Spark 3.5+, so the Spark 3 starter now hard-fails at runtime on Spark 3.3/3.4 clusters with NoSuchMethodError

2. Suggested (non-blocking) follow-ups

Issue 2: apache/spark:3.5.8 runs as non-root user 'spark' (uid 185), which can break file copy/exec of the SeaTunnel starter inside the E2E container
Issue 3: Renaming the root pom property spark.3.3.0.version -> spark.3.5.8.version will break the build of any module still referencing the old property, and bakes the patch version into the property name
Issue 4: Module artifactId still says 'spark-3.3' while the display name and dependencies now target Spark 3.5.8, which is misleading for users and tooling
Issue 5: AbstractTestSparkContainer.startUp() will NPE if a subclass returns null from getDockerCommand()

Nice progress overall — once the blocking points above are addressed this should be in good shape. Happy to discuss.

DanielLeens

Thanks for the ping — I re-reviewed the latest head 3780c41b3ca2 from scratch after the new commits.

What this PR fixes

User pain: The Spark 3 starter and translation layer were pinned to Spark 3.3.0, preventing users from running SeaTunnel jobs on current Spark 3.5.x clusters.
Fix approach: Bump the Spark version property to 3.5.8 across all starter/translation/E2E poms, replace the removed RowEncoder.apply(...) with Encoders.row(...), exclude Spark 3.5's log4j-slf4j2-impl/jul-to-slf4j bridges, switch the E2E container to apache/spark:3.5.8, and add a getDockerCommand() hook to start the Spark master explicitly.
One sentence: The upgrade is mechanically sound and all spark.3.3.0.version references are updated (13 removed, 13 added), but Encoders.row() silently drops runtime compatibility with Spark 3.3/3.4 clusters.

Ingesting other reviewer findings

+1 to @SEZ9's Issue 1 — Encoders.row(StructType) was introduced in Spark 3.5.0 as the replacement for the removed RowEncoder.apply(...). Since Spark is provided scope, the actual Spark classes come from the user's cluster at runtime. Any user deploying the new starter on a Spark 3.3.x or 3.4.x cluster will get a NoSuchMethodError mid-job with no graceful error. I traced the same path at TransformExecuteProcessor.java:180 and confirm this is a real blocker.

I want to add one clarification on @SEZ9's Issue 3: I checked all 13 ${spark.3.3.0.version} references in the current dev branch, and all 13 are updated to ${spark.3.5.8.version} in this PR. So the "remaining references breaking the build" concern does not apply to the current diff — the rename is complete. That said, the suggestion to use a version-agnostic property name like <spark.3.version> is a good non-blocking follow-up to avoid future renames.

I agree with @SEZ9's Issues 2, 4, and 5 as non-blocking follow-ups.

Issue 1 (Blocker): `Encoders.row()` is Spark 3.5+ only, breaks 3.3/3.4 runtime

Location: seatunnel-core/seatunnel-spark-starter/seatunnel-spark-starter-common/src/main/java/org/apache/seatunnel/core/starter/spark/execution/TransformExecuteProcessor.java:180
This is the same blocker @SEZ9 identified. The import changed from RowEncoder.apply(...) (Spark 3.x) to Encoders.row(...) (Spark 3.5+ only). Because Spark is provided scope, users on 3.3/3.4 clusters will hit NoSuchMethodError at runtime when any transform runs — there is no graceful error or fallback.
Suggested fix: either (a) add a reflection-based shim that tries RowEncoder.apply first and falls back to Encoders.row for 3.5+ compatibility, or (b) document this as a breaking change in docs/en/introduction/concepts/incompatible-changes.md (the PR checklist item is unchecked) and fail fast with a clear "Spark >= 3.5 required" error during environment setup.
Severity: High

Merge conclusion

Conclusion: can merge after the blocking item is fixed

Blocking items:

Issue 1: Encoders.row() drops Spark 3.3/3.4 runtime compatibility — needs a reflection shim or explicit incompatible-change documentation + fail-fast guard.

Non-blocking suggestions (agree with @SEZ9):

Issue 2: Pin container user to root or ensure writable paths for apache/spark:3.5.8 (uid 185).
Issue 3: Consider a version-agnostic property name like <spark.3.version> for future-proofing.
Issue 4: The artifactId/directory still says 3.3 — rename or document the minimum Spark version.
Issue 5: Add null guard for getDockerCommand() in AbstractTestSparkContainer.startUp().

Build is pending — will check CI once it completes.

Related to apache#11128 and apache#11307.

goutamadwant · 2026-10-24T17:08:43Z

@SEZ9 @DanielLeens Updated the PR description. Please re-review this PR.

Changes:

Adds a separate Spark 3.5.8 starter and distribution artifacts, preserving the Spark 3.3 path.
Adds runtime row-encoder compatibility, safer Unix argument handling, Windows exit-code handling and packaged logging checks.
Updates connector E2E fixtures and EN/ZH docs, including Spark 3.4 guidance and compatibility limits.
Adds real Spark 3.5.8 batch, bounded micro-batch sink and packaged Unix launcher tests.

Local Java 11 validation on d8045875a0: 22 Spark 3.5 unit tests passed, packaged tests 6 passed/4 Windows-only skips, and 11 adjacent Spark 3.3 tests passed.

Pending: green CI on the merged head, native Windows packaged-launcher execution, and current-head Spark 3.5 connector/deployment validation. Local checks were not rerun after the latest dev merge.

The micro-batch test covers the sink adapter—not streaming through the public starter, checkpoint restore or exactly-once guarantees. Please share any remaining concerns. Thanks.

DanielLeens

Thanks for the new pushes and for refreshing the PR description, @goutamadwant! I re-read the full PR diff on head 28505d7724a9 (against current dev, which is already merged in), and specifically diffed the two new commits on top of the head I last reviewed (5c185476): d8045875 (micro-batch sink test + Unix launcher test + docs) and 28505d77 (a plain merge of dev, no Spark content changes). This is a code-reading review only; nothing was built or executed locally.

What Problem Does This PR Solve?

User pain point: a starter compiled for Spark 3.3 breaks on Spark 3.5.x clusters (RowEncoder.apply(StructType) is gone, and useCommitCoordinator() becomes an ambiguous default method) - #11128.
Fix approach: an opt-in, separate seatunnel-spark-3.5-starter (Spark 3.5.8, provided) that reuses the 3.3 translation layer, with a reflective row-encoder shim, an explicit useCommitCoordinator(), its own .sh (NUL-delimited argv file, no eval) and .cmd launchers, dist packaging, a scheduled-only E2E container and EN/ZH docs.
One-sentence summary: Spark 2.4/3.3, Flink and Zeta paths are untouched; Spark 3.5 users get a documented, batch-first starter.

1. Code Change Review

1.1 Core Logic Analysis

Production code is byte-for-byte what I reviewed on 5c185476 (launchers, SparkStarter, SparkRowEncoder, TransformExecuteProcessor, SeaTunnelBatchWrite). What is new since my last review:

d8045875
  Spark35BatchWriteTest.boundedMicroBatchCommitsRowsThroughStreamingAdapter
    readStream(json, 2 rows) -> map(.. SparkRowEncoder.create(parcelType))
      -> SparkSinkInjector.inject(writeStream, StreamingTestSink, ...) -> Trigger.Once -> start
      -> awaitTermination(60s) -> assert numInputRows==2 and committed "1:first:INSERT","2:second:INSERT"
  TestSpark35StarterJarContentIT.unixLauncherPreservesQuotedArgumentsFromPackagedJvm
    real bash launcher + real starter JVM, fake spark-submit that dumps argv NUL-delimited
    -> asserts --config / --name survive quotes, spaces, $HOME, `id`, $(id); no launcher temp output left
  docs/en+zh/engines/spark.md: scope statements (batch-only public starter, micro-batch test scope)
28505d77  merge dev (44 files total in the PR vs dev, none changed by the merge in Spark paths)

Key findings:

The micro-batch test is a real, failing-capable test: it drives SparkRowEncoder and the streaming sink adapter on Spark 3.5.8 and checks committed values and RowKind. If useCommitCoordinator() or the encoder shim regress on 3.5, it goes red. Good addition, and the docs now describe precisely what it does and does not cover (not the SeaTunnel streaming source, restore or exactly-once).
The Unix launcher test is also meaningful (breaking the argv-file handoff or reintroducing word splitting/eval would fail it). However, see Issue 1: it never runs in CI.

1.2 Compatibility Impact

Fully compatible. Additive starter and launcher; legacy launchers keep their system-property-less defaults; no config, default, serialization or checkpoint change; no new action needed on upgrade.

1.3 Performance / Side-Effect Analysis

No production hot-path change in this delta. The new unit test starts a local Spark session (local[2], UI disabled, loopback bind, Trigger.Once, bounded 60s wait) - a few extra seconds in the Spark 3.5 starter module's unit-test step.

1.4 Error Handling and Logging

Issue 1: The new Unix packaged-launcher test is never executed by any CI lane (carryover of my earlier Issue 6, now more significant)

Location: .github/workflows/backend.yml:709 (Windows unit-test step: -DskipIT=false -Dit.test=TestSpark35StarterJarContentIT); TestSpark35StarterJarContentIT.java:133 (Assumptions.assumeFalse(os.name startsWith "Windows"))
Problem: *IT classes run through failsafe, and the only lane that enables ITs for this module is the Windows one. unixLauncherPreservesQuotedArgumentsFromPackagedJvm is skipped there by its own assumption, and the Linux unit-test lanes use -DskipIT=true. So the Unix quoting/eval-removal guarantee (the main security-flavored claim of this PR, and F2/F6 from SEZ9) is proven only by the author's local run. The PR description also says the 4 Windows-only cases were "expected skips" locally, i.e. no environment executes all 10 cases.
Risk: a future launcher edit can silently reintroduce word splitting or eval with a green CI.
Best fix: run the same IT selection in one Linux lane (e.g. add the -Dit.test=TestSpark35StarterJarContentIT run to one non-Windows job), or move the platform-neutral cases into a surefire *Test (the module already has Spark35LauncherTest).
Severity: Medium (non-blocking - the behavior is covered locally and by review of the script). Raised by another reviewer: partially (SEZ9 asked for evidence of the quoting check; the local run answers that, but not the CI part).

Issue 2: Spark 3.5 connector E2E is scheduled-only; normal PR CI does not exercise it (carryover)

Location: TestContainerId.java:38 (SPARK_3_5(SPARK, "3.5.8", false))
Note: now stated explicitly in spark.md EN/ZH, which is what I asked for. Accepted for this phase. Severity: Low

Issue 3: Windows `.cmd` still hands a single command string to `spark-submit.cmd` (carryover)

Location: start-seatunnel-spark-3.5-connector-v2.cmd:68-79; documented limit. Severity: Low

Issue 4: No compile-time guard for the 3.3-compiled translation layer beyond the scheduled E2E (carryover)

Location: seatunnel-spark-3.5-starter/pom.xml. Severity: Low

Issue 5: `.sh` keeps a parent bash instead of `exec` (carryover)

Location: start-seatunnel-spark-3.5-connector-v2.sh:91; exit status still propagates. Severity: Low

Issue 6: Possible NPE instead of an assertion message if the slf4j manifest has no `Implementation-Version` (carryover nit)

Location: TestSpark35StarterJarContentIT.java:94. Use assertNotNull first. Severity: Low

Issue 7: Micro-batch test nits (new, nit)

Location: Spark35BatchWriteTest.java (boundedMicroBatchCommitsRowsThroughStreamingAdapter)
Problem: (a) Trigger.Once() is deprecated since Spark 3.3 in favor of Trigger.AvailableNow(); (b) SparkSession...getOrCreate() followed by spark.close() would tear down a session shared with any other test in the same JVM. Today Spark35RowEncoderTest does not use a session, so there is no actual problem now.
Severity: Low

2. Code Quality Assessment

2.1 Coding Standards: fine; comments explain the "why" (capture-at-spark-submit boundary, streaming-adapter-not-public-starter).
2.2 Test Coverage and Test Stability: Stable. No sleeps or fixed ports; loopback bind, @TempDir isolation, bounded awaitTermination(60000), finally stops the query and closes the session. Coverage gap: Issue 1.
2.3 Documentation Updates: EN and ZH are in sync for the new scope statements and the Unix test description.

3. Architectural Soundness

3.1 Elegance: Precise fix - isolated, additive starter.
3.2 Maintainability: good; launcher protocol and limits are documented.
3.3 Extensibility: a dedicated 3.5 translation module is the natural follow-up if Catalyst drift appears (Issue 4).
3.4 Historical-Version Compatibility: Fully compatible.

4. Issue Summary

No.	Issue	Location	Severity
1	Unix packaged-launcher test never runs in CI	`backend.yml:709`, `TestSpark35StarterJarContentIT.java:133`	Medium (non-blocking)
2	Spark 3.5 connector E2E scheduled-only (documented)	`TestContainerId.java:38`	Low
3	Windows `.cmd` single command string	`start-seatunnel-spark-3.5-connector-v2.cmd:68-79`	Low
4	No compile guard for the 3.3-compiled translation layer	`seatunnel-spark-3.5-starter/pom.xml`	Low
5	`.sh` without `exec`	`start-seatunnel-spark-3.5-connector-v2.sh:91`	Low
6	NPE instead of assertion message	`TestSpark35StarterJarContentIT.java:94`	Low
7	`Trigger.Once` deprecated; shared-session close	`Spark35BatchWriteTest.java`	Low

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Strictly, nothing here is a source-level blocker; the "after fixes" is for the CI gate below, and I would treat Issue 1 as a strongly recommended follow-up.

Blockers (must fix):

CI for this exact head is not finished. The fork Build run for 28505d77 is queued, and the apache-side Build pointer is IN_PROGRESS. The two previous heads' fork runs were both cancelled (superseded by the newer pushes), so there is currently no completed CI result for any head containing the new tests. Please let this run finish green; in particular both unit-test (*, windows-latest) jobs should show TestSpark35StarterJarContentIT executing the Windows-only cases (not skipped), and the Spark 3.5 starter unit tests should show Spark35BatchWriteTest passing.

Recommended fixes (non-blocking): Issue 1 first, then Issues 2-7 as you see fit.

Merge-state facts: the branch is up to date with dev (ahead_by=67, behind_by=0), no conflicts; mergeStateStatus=BLOCKED only because required checks and a write-access approval are pending. No new maintainer CHANGES_REQUESTED is outstanding on this head: SEZ9's earlier F1-F8 are all addressed in code/docs per my prior re-checks (F4/F7/F8 closed on 5c185476); @SEZ9 may want to confirm the Windows run and the new micro-batch test once CI is green. My account is read-only, so a committer still has to give the final approval.

Overall assessment: the new commits close the "no streaming/adapter coverage" and "no quoting evidence" concerns with honest, well-scoped tests and documentation. The remaining risk is about where those tests run, not about their correctness. Thank you for your patience on this long iteration!

SEZ9 · 2026-10-25T02:25:10Z

Thanks @goutamadwant for the update and the summary. Since I can only confirm fixes from the diff itself, here is what I'd like to pin down for each earlier point:

F1 / F5 (Windows launcher) – "Windows exit-code handling" sounds like F5. Could you confirm F1 is also fixed, i.e. setlocal disabledelayedexpansion no longer runs before !errorlevel!/!CMD! are expanded, so the generated command is actually executed? Since the packaged tests have 4 Windows-only skips and native Windows execution is still pending, a pasted run from a real Windows box (success path plus a forced non-zero exit from the starter java process) would settle both.
F2 / F6 (Unix launcher) – "safer Unix argument handling" sounds like F6 (quoting). Is the command still run through eval (F2), or executed directly now? If eval is still there I'd like it removed before merge.
F3 (shaded 3.3 translation layer) – does the Spark 3.5 starter still shade the Spark 3.3-compiled translation layer? If so, what does the runtime row-encoder compatibility change cover, and which Catalyst incompatibilities remain that the documented compatibility limits call out?
F4 (logging jars) – what do the "packaged logging checks" assert? I'm specifically looking for a check that only one of log4j-slf4j-impl / log4j-slf4j2-impl ends up on the starter/logging/* classpath for each starter.
F7 (streaming coverage) – appreciate the honesty that the micro-batch test covers the sink adapter rather than streaming through the public starter. That's acceptable for this PR provided the docs say the same — could you point me to where the Spark 3.5 docs tell users the streaming template is not validated end-to-end on 3.5.8 yet?
F8 (Spark 3.4 guidance) – could you point me to the EN/ZH doc text that now covers Spark 3.4.x users?

Before merge I'd also want green CI on the merged head, since local checks weren't rerun after the latest dev merge, plus the Windows run above. Once those are in I'm happy to do the final re-review. Thanks for the thorough work on this.

Signed-off-by: Goutam Adwant <workwithgoutam@gmail.com>

DanielLeens

Thanks for the quick follow-up, @goutamadwant! The head moved from 28505d77 to 380764cb by one commit that touches only .github/workflows/backend.yml (+5 lines). I re-read it against my previous review and the surrounding job. Review is by code reading only; nothing was executed locally.

What Problem Does This PR Solve?

User pain point: a starter compiled for Spark 3.3 breaks on Spark 3.5.x clusters (RowEncoder.apply(StructType) is gone and useCommitCoordinator() becomes an ambiguous default method) - #11128.
Fix approach: an opt-in, separate seatunnel-spark-3.5-starter (Spark 3.5.8, provided) that reuses the 3.3 translation layer, with a reflective row-encoder shim, an explicit useCommitCoordinator(), its own .sh (NUL-delimited argv file, no eval) and .cmd launchers, dist packaging, a scheduled-only E2E container and EN/ZH docs.
One-sentence summary: Spark 2.4/3.3, Flink and Zeta paths are untouched; Spark 3.5 users get a documented, batch-first starter.

1. Code Change Review

1.1 Core Logic Analysis

All production code and tests are unchanged from the head I reviewed in full. The new commit adds one step to the unit-test job in .github/workflows/backend.yml (around line 716):

- name: Run packaged Spark 3.5 launcher tests (Unix)
  if: runner.os != 'Windows'
  run: ./mvnw -B -T 1 verify -pl seatunnel-core/seatunnel-spark-starter/seatunnel-spark-3.5-starter -am
       -DskipUT=true -DskipIT=false -D"it.test=TestSpark35StarterJarContentIT" ...

This closes my previous Issue 1 (the Unix packaged-launcher test unixLauncherPreservesQuotedArgumentsFromPackagedJvm ran in no CI lane): the job matrix is ubuntu-latest and windows-latest x JDK 11/17, the Windows lane already ran this IT selection, and the new Unix step now runs it on both Ubuntu lanes. The test skips itself on Windows via Assumptions.assumeFalse(os.name startsWith "Windows") (TestSpark35StarterJarContentIT.java:133), so the two steps are complementary and between them all cases can run in CI. it.test is the failsafe selection property and skipIT defaults to true in the root pom (pom.xml:190), so -DskipIT=false is required and present.

Key findings:

The quoting/no-eval guarantee of the Unix launcher (the F2/F6 concern from @SEZ9) is now protected by CI, not only by a local run.
The step uses -am, so it rebuilds the upstream modules once more after the earlier clean verify. This only costs CI time; it is not a correctness issue (see Issue 1).
Step ordering is safe: it runs after the Windows/Unix unit-test steps and before the report upload, which has if: failure(), so a failure still uploads reports.

1.2 Compatibility Impact

Fully compatible. CI-only change on this head; the PR as a whole is additive (new starter and launchers, no change to Spark 2.4/3.3, Flink or Zeta paths, no config, default, serialization or checkpoint change).

1.3 Performance / Side-Effect Analysis

No runtime effect. CI cost: one extra Maven invocation per Ubuntu unit-test lane (-am re-verifies upstream modules with unit tests skipped, then runs one IT class), so it adds some minutes to the two Ubuntu lanes.

1.4 Error Handling and Logging

Issue 1 (Low, non-blocking): the new step rebuilds all upstream modules

Location: .github/workflows/backend.yml new step, -pl ... -am
Problem: after the preceding clean verify has already built everything, -am repeats the packaging of the whole upstream chain.
Best fix: acceptable as is. If lane time matters, -o/reuse of the built reactor, or running the IT in the same Maven invocation as the main step with an -Dit.test filter, would save the rebuild. Not required for this PR.
Severity: Low. Raised by another reviewer: No

The remaining low-severity carryovers from my last review are unchanged: scheduled-only Spark 3.5 connector E2E (now documented, TestContainerId.java:38), Windows .cmd passes a single command string (documented limit), no compile guard for the 3.3-compiled translation layer, .sh keeps a parent bash instead of exec, a possible NPE at TestSpark35StarterJarContentIT.java:94 when the slf4j manifest has no Implementation-Version, and the Trigger.Once deprecation / session close nits in Spark35BatchWriteTest.

2. Code Quality Assessment

2.1 Coding Standards

Fine; the workflow step is consistent with the neighboring Windows step.

2.2 Test Coverage and Test Stability

Rating: Stable. No tests changed on this head. Coverage gap from last round is closed by the new step.

2.3 Documentation Updates

EN and ZH docs are in sync for the new scope statements. Nothing new is required for a CI-only change.

3. Architectural Soundness

3.1 Elegance

Precise fix - isolated, additive starter.

3.2 Maintainability

Good; launcher protocol and limits are documented.

3.3 Extensibility

A dedicated 3.5 translation module is the natural follow-up if Catalyst drift appears.

3.4 Historical-Version Compatibility

Fully compatible.

4. Issue Summary

Number	Issue	Location	Severity
1	New Unix IT step re-runs upstream build via `-am`	`.github/workflows/backend.yml` (new step)	Low
2	Spark 3.5 connector E2E scheduled-only (documented)	`TestContainerId.java:38`	Low
3	Windows `.cmd` single command string	`start-seatunnel-spark-3.5-connector-v2.cmd:68-79`	Low
4	No compile guard for the 3.3-compiled translation layer	`seatunnel-spark-3.5-starter/pom.xml`	Low
5	`.sh` without `exec`	`start-seatunnel-spark-3.5-connector-v2.sh:91`	Low
6	NPE instead of assertion message	`TestSpark35StarterJarContentIT.java:94`	Low
7	`Trigger.Once` deprecated; shared-session close	`Spark35BatchWriteTest.java`	Low

5. Merge Recommendation

Conclusion: Ready to merge after fixes

There is no source-level blocker; the "after fixes" is only the CI gate.

Blockers (must fix)

CI on this exact head has not finished: the apache Build pointer is in_progress. Earlier heads' fork runs were cancelled by newer pushes, so no completed run yet covers the new tests. Please let this one finish green; in particular the new "Unix" step should show TestSpark35StarterJarContentIT executing on both Ubuntu lanes, the Windows lanes should execute the Windows-only cases rather than skip them, and Spark35BatchWriteTest should pass.

Recommended fixes (non-blocking): Issues 1-7 as you see fit.

Merge state: the branch is 3 commits behind dev, mergeable with no conflicts; no sync is needed. mergeStateStatus is BLOCKED only because required checks and a write-access approval are pending. @SEZ9's latest comment asks for confirmation of the F1-F8 points and a green merged-head CI; the CI part is exactly the blocker above, and the F1-F8 items were re-checked in my previous reviews against the code and docs (F1, F2, F6 launcher behavior; F4, F7, F8 closed earlier). A committer still has to give the final approval since my account is read-only.

Overall assessment: the series is in good shape and this last commit closes the only meaningful coverage gap. The remaining work is waiting for CI. Thank you for your patience on this long iteration!

DanielLeens · 2026-10-26T08:30:19Z

Quick follow-up on my review of 380764cb (the one I posted while the apache Build check was still queued). Thanks for your patience, @goutamadwant. That statement is now stale: the check finished with failure on this same head, so the "only the CI gate is left" line needs a correction. Review is by log reading only; nothing was executed locally.

What the CI says (head `380764cb`, unchanged since my review)

The apache Build check is a pointer to the fork run: run 40159933537. Failing jobs:

Job	Failure	Related to this PR?
`unit-test (11, windows-latest)` and `unit-test (17, windows-latest)`	`Spark35BatchWriteTest.boundedMicroBatchCommitsRowsThroughStreamingAdapter:103` -> `UnsatisfiedLinkError: 'boolean org.apache.hadoop.io.nativeio.NativeIO$Windows.access0(java.lang.String, int)'`	Yes - the test was added by this PR (commit `d8045875a07`)
`engine-v2-it (17, ubuntu-latest)`	`SplitClusterFaultToleranceIT.testStreamJobCancelResolvesWhenWorkerCrashesBeforeCancelAck` -> `ConditionTimeout` (1 error of 215 tests)	No - Zeta cancel-after-worker-loss flake, unrelated to the Spark starter
`connector-file-local-it` (11 and 17)	cancelled	No - cancelled by the failing jobs, no test result

Both Ubuntu unit-test lanes are green, so the new test passes on Linux; it only breaks on the Windows runners.

1. Windows unit-test failure (needs a small change in this PR)

The new test starts a real local SparkSession and a file-based Trigger.Once streaming query with a checkpoint directory. On Windows, Hadoop's local file system needs the native winutils/hadoop.dll layer for the permission check, which the runner does not provide, hence the UnsatisfiedLinkError at Spark35BatchWriteTest.java:103 (the .start() path). This is an environment limitation of the Windows runner, not a bug in the production code.

Minimal next step: skip this one test on Windows, exactly as you already did for the launcher test in Spark35LauncherTest.java:48:

@Test
public void boundedMicroBatchCommitsRowsThroughStreamingAdapter() throws Exception {
    Assumptions.assumeFalse(System.getProperty("os.name").startsWith("Windows"));
    ...
}

(plus import org.junit.jupiter.api.Assumptions;). The Linux lanes keep running it, so the streaming-adapter regression guard stays in CI. Alternatively, provide winutils on the Windows lane, but that is a much bigger CI change for one test.

2. `SplitClusterFaultToleranceIT` is a known engine flake

That test fails when a worker is lost while a cancel is in flight (the job ends up in the wrong terminal state). It is unrelated to anything in seatunnel-spark-3.5-starter. dev has a fix, #12377 "Preserve cancellation after worker failure", merged on 2026-10-26 and not yet in your branch (your branch is 11 commits behind dev). Once you update from dev, a re-run of that job should pass; there is no need to change anything in this PR for it.

Source conclusion (unchanged)

There is still no source-level blocker in the production code; my earlier review of 380764cb stands (Fully compatible, only the Low carryovers). One addition to Issue 7 of my last review (the Spark35BatchWriteTest nits): the Windows guard above is now the one concrete item for that test.

Conclusion: Ready to merge after fixes

Blocker (CI only): add the Windows Assumptions guard in Spark35BatchWriteTest, then update the branch from dev and let the Build check re-run (job-level re-run of SplitClusterFaultToleranceIT is fine if it recurs).
No other changes are requested. Thanks again for the careful work on this one!

Signed-off-by: Goutam Adwant <workwithgoutam@gmail.com>

DanielLeens

Thanks for the quick follow-up, @goutamadwant, and for picking up the Windows suggestion so fast. The head moved from 380764cb to 4b80eba2 by exactly one commit, [Test][Core] Guard Spark file-stream test on Windows. I compared that commit against my previous review and against the failing Windows logs. Static source review only; CI is the runtime evidence.

What Problem Does This PR Solve?

User pain point: a starter compiled for Spark 3.3 breaks on Spark 3.5.x clusters (RowEncoder.apply(StructType) is gone and useCommitCoordinator() becomes an ambiguous default method), see #11128.
Fix approach: an opt-in, separate seatunnel-spark-3.5-starter (Spark 3.5.8, provided) that reuses the 3.3 translation layer, with a runtime row-encoder shim, an explicit useCommitCoordinator(), its own .sh (NUL-delimited argv file, no eval) and .cmd launchers, dist packaging, a scheduled-only E2E container and EN/ZH docs.
One-sentence summary: Spark 2.4/3.3, Flink and Zeta paths are untouched; Spark 3.5 users get a documented, batch-first starter.

1. Code Change Review

1.1 Core Logic Analysis

Only one file changed on this head, and it is a test:

// Spark35BatchWriteTest.java
@Test
public void boundedMicroBatchCommitsRowsThroughStreamingAdapter() throws Exception {
+   Assumptions.assumeFalse(
+           System.getProperty("os.name").startsWith("Windows"),
+           "Spark's local file stream requires Hadoop native libraries on Windows");
    Path input = Files.createDirectory(tempDir.resolve("input"));

This is exactly the change I suggested after the previous head failed on both unit-test (11/17, windows-latest) jobs with UnsatisfiedLinkError: NativeIO$Windows.access0 at Spark35BatchWriteTest.java:103. The root cause was the Hadoop local file system permission check on a Windows runner without winutils/hadoop.dll, triggered by the file-based Trigger.Once query and its checkpoint directory. It is an environment limitation, not a production bug. The guard uses the same os.name idiom already used for the launcher test (Spark35LauncherTest.java:48) and TestSpark35StarterJarContentIT.java:133, and Assumptions is imported correctly (new import in the diff).

Runtime path text flow (unchanged by this commit, rechecked for context):

Spark35BatchWriteTest (surefire, unit-test lane)
  -> local SparkSession -> readStream(file) -> SparkRowEncoder -> SeaTunnelBatchWrite streaming adapter
  -> writeStream.trigger(Once).option("checkpointLocation", tempDir) -> .start()
       Ubuntu: passes (previous run green on both Ubuntu lanes)
       Windows: Hadoop RawLocalFileSystem -> NativeIO$Windows.access0 -> UnsatisfiedLinkError (now skipped by assumption)

Key findings:

The skip is narrowly scoped to the single test that needs the native layer; the batch-write, row-encoder and launcher tests still run on Windows.
The micro-batch sink adapter stays covered on both Ubuntu lanes (JDK 11 and 17), so the coverage that matters for the Spark 3.5 sink path is not lost.
No production code, pom, doc, launcher or workflow file is touched.

1.2 Compatibility Impact

Fully compatible. Test-only change on this head; the PR as a whole remains additive (new starter, launchers and dist entries, no change to Spark 2.4/3.3, Flink or Zeta paths, no config, default, serialization or checkpoint change).

1.3 Performance / Side-Effect Analysis

No runtime effect. Test-only, and it only reduces work on the Windows lanes.

1.4 Error Handling and Logging

No new issues from this commit. One item is carried over from my previous review because the CI state is still the gate:

Issue 1 (Medium, CI gate, not a source defect): the head has no completed green CI yet

Location: apache Build check run 122671703070 (pointer to fork run 40644985034), status in_progress at the time of review
Problem: the previous head was red on the two Windows unit-test jobs (fixed by this commit) plus an unrelated Zeta flake SplitClusterFaultToleranceIT.testStreamJobCancelResolvesWhenWorkerCrashesBeforeCancelAck in engine-v2-it (17, ubuntu-latest). We need this head's run to finish before drawing a conclusion.
Potential risk: if the Windows lane still fails on a different Hadoop-native path (for example another test that starts a local Spark file stream), the fix would need one more round.
Best improvement: let the run finish; if only SplitClusterFaultToleranceIT or other Zeta/connector jobs fail, rerun only the failed jobs. The branch is also 44 commits behind dev, so a sync is advisable before the final green run.
Severity: Medium (gate). Raised by another reviewer: Yes, @SEZ9 asked for green CI on the merged head and a Windows run.

The remaining Low carryovers are unchanged and non-blocking: Spark 3.5 connector E2E is scheduled-only (documented in docs/en/engines/spark.md and at TestContainerId.java:38), the Windows .cmd passes a single command string (documented limit), no compile guard for the 3.3-compiled translation layer, .sh keeps a parent bash instead of exec, a possible NPE at TestSpark35StarterJarContentIT.java:94 when the slf4j manifest has no Implementation-Version, the extra -am rebuild in the Unix IT step in backend.yml, and the Trigger.Once deprecation / shared-session close nits in Spark35BatchWriteTest.

Existing reviewer threads

@SEZ9 (latest comment on 28505d77/380764cb) asked for: Windows launcher expansion and exit-code proof, eval removal, the 3.3 translation-layer shading rationale, a logging-binding assertion, docs on streaming coverage and Spark 3.4 guidance, and green CI. Against the current head: eval is gone (NUL-delimited argv handoff in start-seatunnel-spark-3.5-connector-v2.sh); the .cmd no longer relies on delayed expansion and propagates the exit code; the packaged IT asserts a single SLF4J binding (TestSpark35StarterJarContentIT checks for org/apache/logging/slf4j/ and no org/slf4j/impl/StaticLoggerBinder.class); docs/en/engines/spark.md now states that streaming through the public starter is not validated and that Spark 3.4 users keep seatunnel-spark-3-starter.jar (ZH docs match). The two items that only CI can settle are native Windows execution of the packaged cases and green CI on this head. SEZ9 should refresh the formal review state (the last formal review is still CHANGES_REQUESTED on 3780c41b), since GitHub still reports the PR as blocked.
My earlier CHANGES_REQUESTED reviews were resolved by later heads; nothing from them is outstanding.
@liunaijie's earlier comments were about scope and layout and were addressed by the separate-starter design.

2. Code Quality Assessment

2.1 Coding Standards

Fine. The guard follows the existing assumption idiom, has a clear message and an unused-import-free diff. The core test methods in this PR carry no regressions in style.

2.2 Test Coverage and Test Stability

Rating: Stable for this commit. The assumeFalse removes the only known OS-environment dependency; the test still runs on both Ubuntu lanes. The assumption is evaluated before any Spark or file-system work, so it cannot leave partial state behind. Note that a skipped-by-assumption test is reported as skipped, not passed, on Windows, which matches what the docs say about Windows coverage.

2.3 Documentation Updates

EN and ZH are in sync for the scope, streaming and Spark 3.4 statements. Nothing new is required for a test-only change.

3. Architectural Soundness

3.1 Elegance

Precise fix. A one-test skip for a known Windows runner limitation, mirroring existing practice in this PR.

3.2 Maintainability

Good; the skip reason is written in the assumption message.

3.3 Extensibility

A dedicated 3.5 translation module is the natural follow-up if Catalyst drift appears.

3.4 Historical-Version Compatibility

Fully compatible.

4. Issue Summary

Number	Issue	Location	Severity
1	Head `4b80eba2` has no completed green CI yet; branch is 44 commits behind `dev`	apache `Build` check 122671703070	Medium (CI gate)
2	Spark 3.5 connector E2E scheduled-only (documented)	`TestContainerId.java:38`	Low
3	Windows `.cmd` passes a single command string (documented)	`start-seatunnel-spark-3.5-connector-v2.cmd:68-79`	Low
4	No compile guard for the 3.3-compiled translation layer	`seatunnel-spark-3.5-starter/pom.xml`	Low
5	`.sh` without `exec`	`start-seatunnel-spark-3.5-connector-v2.sh:91`	Low
6	NPE instead of assertion message	`TestSpark35StarterJarContentIT.java:94`	Low
7	`Trigger.Once` deprecated; shared-session close	`Spark35BatchWriteTest.java`	Low
8	Unix IT step rebuilds upstream via `-am`	`.github/workflows/backend.yml`	Low

5. Merge Recommendation

Conclusion: Ready to merge after fixes

There is no source-level blocker on this head. The "after fixes" is only the CI gate and housekeeping.

Blockers

CI: the Build run for 4b80eba2 must finish green, in particular both unit-test (*, windows-latest) jobs that failed on the previous head.

Recommended fixes (non-blocking)

Sync with dev (44 commits behind) before the final green run; if only unrelated Zeta or connector jobs fail, rerun just those jobs.
Ask @SEZ9 to refresh the formal review state once CI is green.
Optionally address the Low items above in a follow-up.

Overall assessment: the single new commit is the minimal, correct response to the Windows failure, and the earlier launcher, logging, docs and coverage concerns are closed in the source. Once the Windows lanes turn green on this head, I would be comfortable approving.

Alternatives considered: skipping via a Surefire exclusion on Windows, or providing winutils on the runner. The in-test assumption is simpler and keeps the test visible in reports, so I think it is the better choice.

DanielLeens · 2026-10-28T15:59:44Z

Thanks for the contribution. The CI fact cited in my previous conclusion has changed; this follow-up updates that gate on the unchanged reviewed head, without issuing another full source review.

Gate snapshot (2026-10-28T15:59:39+00:00), head 4b80eba2519f8a1d13a54229dbd2090d7fca8a9f: Build is FAILURE.

The blocking jobs are Run / unit-test (17, windows-latest) failure; Run / unit-test (11, windows-latest) failure; Run / all-connectors-it-3 (17, ubuntu-latest) cancelled; Run / all-connectors-it-3 (11, ubuntu-latest) cancelled; Run / connector-file-local-it (11, ubuntu-latest) cancelled.

Observed log evidence: Failed to execute goal org.apache.maven.plugins:maven-failsafe-plugin:2.22.2:integration-test (default) on project seatunnel-resource-managers-e2e: No tests were executed! (Set -DfailIfNoTests=false to ignore this error.) -> [Help 1]

Please inspect or rerun only the named failed job where a rerun is appropriate. The logs establish the failing stage, but do not alone establish whether the current PR caused it. No matching upstream cure has been proved by this follow-up; a base sync alone is not a verified resolution, so no unrelated module/dependency patch is requested.

My previous source conclusion remains clear for the reviewed scope. CI is not being passed by inference, and this follow-up is not an effective approval.

Comparison recorded in this scan: base dev, diverged, ahead 69 / behind 49, no merge conflict. Branch drift is metadata here and is not an additional sync request.

SEZ9 · 2026-10-29T04:24:15Z

Quick follow-up on CI for head 4b80eba2519f8a1d13a54229dbd2090d7fca8a9f: the Build run is FAILURE. Blocking jobs: Run / unit-test (17, windows-latest) failure, Run / unit-test (11, windows-latest) failure, plus the cancelled Run / all-connectors-it-3 (17, ubuntu-latest), Run / all-connectors-it-3 (11, ubuntu-latest) and Run / connector-file-local-it (11, ubuntu-latest). The log shows maven-failsafe-plugin:2.22.2:integration-test (default) on project seatunnel-resource-managers-e2e: No tests were executed!, which identifies the failing stage but doesn't by itself show whether this PR caused it.

Asks:

Please take a look at the two Windows unit-test failures and the seatunnel-resource-managers-e2e failsafe error, and either rerun those jobs if a rerun is appropriate or post a short note on the root cause. Please avoid unrelated module/dependency changes unless you've confirmed they are the actual fix.
The points from the previous review still apply to this head as far as I can see; let me know if any have already been addressed:
- PR11307-F1 (HIGH): in start-seatunnel-spark-3.5-connector-v2.cmd, setlocal disabledelayedexpansion runs before !errorlevel!/!CMD! are expanded, so the call target is the literal text !CMD! and the job never runs.
- PR11307-F5: the same .cmd exits 0 when the starter java process fails with no stdout, and for /f hides java's real exit code.
- PR11307-F2 / F6: in start-seatunnel-spark-3.5-connector-v2.sh, the eval over the assembled command line executes config-controlled content, and args=$@, ${args} and ${CLASS_PATH} are unquoted.
- PR11307-F3: the 3.5 starter shades seatunnel-translation-spark-3.3; please describe how Catalyst binary compatibility against Spark 3.5.8 was verified.
- PR11307-F4: both log4j-slf4j-impl and log4j-slf4j2-impl land in starter/logging/*, which both launchers put on the classpath.
- PR11307-F7: the docs point Spark 3.5 users at the streaming template, but the new tests only cover the row encoder and batch write.
- PR11307-F8: docs/en/engines/spark.md narrows seatunnel-spark-3-starter.jar to "Spark 3.3.x" and leaves 3.4.x users without guidance.

Once the Windows launcher (F1/F5) and shell quoting/eval items (F2/F6) are fixed and the named jobs are green, I'll take another pass. Thanks for the work on this.

DanielLeens · 2026-10-29T17:14:52Z

Thanks @SEZ9 for making the concerns explicit. I fetched and rechecked the exact current head against those points. Several describe earlier versions: the Windows launcher now runs java directly and saves %errorlevel% before FOR /F (start-seatunnel-spark-3.5-connector-v2.cmd:53-64), then calls spark-submit.cmd with %CMD% at :79; it no longer invokes literal !CMD! or loses the java status. The Unix launcher uses a NUL-separated argument file, arrays and quoted argv at .sh:73-91, with no eval. The new starter POM excludes log4j-slf4j2-impl; TestSpark35StarterJarContentIT:75-88 checks exactly one SLF4J 1.7 binding and no SLF4J 2 provider. docs/en/engines/spark.md:19-39 retains 3.4 guidance, limits the public 3.5 launcher to batch, explains the bounded micro-batch test and explicitly states the remaining Catalyst/native-Windows validation limits. Reusing the 3.3 translation layer is therefore a documented, bounded compatibility claim, not proof of every 3.5 path. I am not reopening the addressed source findings from stale snippets. CI still needs resolution: both Windows unit steps stop in seatunnel-resource-managers-e2e with No tests were executed, and three connector/file shards are cancelled. Latest dev has the same resource-manager Failsafe selection and no verified fix for that exact failure; a sync alone is not a demonstrated cure. Have the CI owner inspect the skipIT/test-selection propagation and rerun the named failed/cancelled jobs; do not change the repaired Spark launcher speculatively to chase that unrelated stage.

Checked 2026-10-29T17:14:45+00:00; head 4b80eba2519f8a1d13a54229dbd2090d7fca8a9f. Signal pull request update = SUCCESS; Build = FAILURE

Comparison: base dev, head 4b80eba2519f8a1d13a54229dbd2090d7fca8a9f, diverged, ahead 69 / behind 61; GitHub mergeable state blocked. Behind/diverged alone is recorded metadata and does not add a sync request.

Daniel has READ repository permission. This is a discussion/CI clarification, not a new complete source review, an effective APPROVED event or dismissal of historical requested changes. A write-capable maintainer must perform any required final approval/dismissal. Thanks for keeping the follow-up focused.

SEZ9 · 2026-10-30T03:15:02Z

Thanks for the detailed re-check against head 4b80eba2519f8a1d13a54229dbd2090d7fca8a9f. I have not yet confirmed those changes in the diff myself, so I'm leaving F1–F8 open until I do rather than closing them from the description alone. To speed up that pass, it would help if the PR author could point me to the relevant hunks for:

F1 / F5 (start-seatunnel-spark-3.5-connector-v2.cmd): where the java exit code is captured and propagated, and how the empty-stdout case now fails rather than exiting 0.
F2 / F6 (start-seatunnel-spark-3.5-connector-v2.sh): the argument handling that replaced eval and the unquoted args/CLASS_PATH usage.
F4 (seatunnel-core/seatunnel-spark-starter/pom.xml): the logging-jar include/exclude change and the test that asserts a single SLF4J binding.
F3 (seatunnel-spark-3.5-starter/pom.xml): the docs wording that explicitly names Spark 3.5 when describing the remaining Catalyst validation limits of the reused 3.3 translation layer.
F7 (docs/en/getting-started/locally/quick-start-spark.md): confirmation that the quick-start page no longer sends Spark 3.5 users to the streaming template, plus the bounded micro-batch test that runs against 3.5.8.
F8 (docs/en/engines/spark.md): the retained Spark 3.4.x guidance.

On CI: Build = FAILURE and Signal pull request update = SUCCESS. The No tests were executed stop in seatunnel-resource-managers-e2e and the cancelled connector/file shards don't appear related to the launcher/POM/doc items above, so I agree the Spark launcher should not be changed speculatively to chase that. Those jobs still need to be green (or shown to be a pre-existing dev issue) before merge, but that's outside this review's scope.

Once I've read the head against the points above, I'll post a final pass.

DanielLeens · 2026-10-30T03:31:35Z

Thanks @SEZ9. Here are current-head pointers for that pass: the 3.5 .cmd launcher captures the Java status at :55, propagates nonzero at :61-64, rejects missing/empty command output at :68-77 and propagates spark-submit at :79-80. The .sh launcher uses quoted "$@" and classpath at :75 and an argument array at :91, without eval. The parent starter pom excludes embedded bindings at :85-95; TestSpark35StarterJarContentIT.java:75-88 requires exactly one external SLF4J 1.7 binding and no SLF4J 2 provider. docs/en/engines/spark.md:19-33 retains 3.4 guidance and names the reused 3.3 translation/Catalyst boundary. The quick-start at :30-33/:111-114 directs 3.5 to batch; Spark35BatchWriteTest.boundedMicroBatchCommitsRowsThroughStreamingAdapter:73-135 separately asserts termination, two input rows and exact committed records on Spark 3.5.8. That test does not establish public-starter streaming/checkpoint recovery. These points support my existing source clearance, not an attribution of runtime proof to local execution. The actual resource-manager Failsafe "No tests were executed" and cancelled CI shards still need resolution; latest dev has no matching verified fix, so a simple sync or launcher change is not prescribed as their cure.

Checked 2026-10-30T03:30:37.152172+00:00; head 4b80eba2519f8a1d13a54229dbd2090d7fca8a9f. Signal pull request update=SUCCESS; Build=FAILURE. Base dev, diverged, ahead 69/behind 64; merge blocked.

This is an unchanged-head discussion/CI clarification, not another full source review or an effective approval. Daniel has READ repository permission.

Signed-off-by: Goutam Adwant <workwithgoutam@gmail.com>

DanielLeens

Thanks for retaining the separate starter and making the validation limits explicit. I reviewed the complete 44-file diff at 5184b37, including both launchers, packaging, shared Spark callers, test-container registration and current CI logs. The earlier F1-F8 launcher/logging/documentation concerns are addressed in the source. Both native-Windows unit lanes and their packaged-launcher step now pass. Build is still red, and there is now a specific upstream checkpoint-lock fix relevant to the failed engine test; the previous statement that no matching upstream fix exists no longer applies.

What Problem Does This PR Solve?

User pain point: The Spark 3.3 starter hits removed RowEncoder APIs and a conflicting DataSource V2 default method when used with Spark 3.5.

Fix approach: Add an opt-in Spark 3.5.8 starter and scripts, resolve row encoding at runtime, explicitly implement the shared commit-coordinator default, package the new artifact alongside existing starters and register a scheduled Spark 3.5 connector container.

One-sentence summary and example: A Spark 3.5 batch user selects start-seatunnel-spark-3.5-connector-v2.sh; the JVM writes raw NUL-delimited arguments and Bash passes them as an array to the matching Spark installation.

1. Code Change Review

1.1 Core Logic Analysis

The new module pins Spark streaming/core/sql 3.5.8 with the inherited provided scope and Scala 2.12, while reusing the Spark 3.3 translation jar. SparkRowEncoder looks up Encoders.row first and falls back only when that method is absent, preserving the original invocation failure cause rather than trying another encoder after a real failure. TransformExecuteProcessor uses that helper. SeaTunnelBatchWrite explicitly returns true for useCommitCoordinator, resolving the two Spark 3.5 defaults while preserving the old batch default. SparkStarter selects jar/help names through launcher properties and enables raw arguments only for the args-file property; default legacy command formatting remains.

Before:

ExpressionEncoder<Row> encoder = RowEncoder.apply(schema);
// 3.3-compiled BatchWrite + StreamingWrite inherits the older API shape.

After:

Encoder<Row> encoder = SparkRowEncoder.create(schema);
@Override public boolean useCommitCoordinator() { return true; }
// Unix launcher reads raw NUL-delimited items and invokes a quoted argv array.

Key Findings

Unix Java options preserve historical whitespace splitting with glob expansion disabled; user CLI arguments and the classpath are quoted. No eval remains.
The temporary argv file is created with mktemp and removed by EXIT; nonzero Java status, help, empty successful output and Spark status have explicit handling.
Windows runs Java directly, captures its status before FOR /F, rejects empty command output and propagates spark-submit status. Its final command-string boundary remains a documented restricted interface.
The launcher JVM uses external SLF4J 1.7 logging; the starter shade excludes both Log4j SLF4J bindings and Scala classes. Spark submission uses its own runtime.
Both CI/full distribution descriptors include the new scripts and starter; old starter modules/artifacts remain.
The container is AutoService registered, has an explicit Apache Spark master command, and is testInPR=false. The shared container default stays unchanged.
Hudi/Iceberg stage Hadoop 3.3.4 S3 support only into Spark 3.5; existing engine paths keep their previous dependencies. Hudi buckets are separated by runtime; all callers of its now-parameterized fixture pass a bucket. Kafka offsets and Python container selection include Spark 3.5. Druid fixture scale now matches its eight-decimal sample values.
The public pipeline still uses read() and write().save(); the bounded micro-batch test exercises the sink adapter separately.

Complete runtime path

Unix/Windows launcher -> Java SparkStarter -> parse CLI and job env
 -> client/cluster-specific flags -> discover plugin jars -> buildFinal
 -> Unix UTF-8 NUL argv / Windows legacy command text -> spark-submit
 -> SeaTunnelSpark -> SourceExecuteProcessor batch Dataset
 -> TransformExecuteProcessor -> runtime SparkRowEncoder -> transformed Dataset
 -> SinkExecuteProcessor -> SparkSinkInjector -> SeaTunnelBatchWrite
 -> per-partition writer -> commit messages -> aggregated committer
E2E ServiceLoader -> SPARK_3_5 when TEST_IN_PR=false -> Apache Spark 3.5.8
 -> staged S3 dependencies where needed -> same packaged launcher

The raw path splits -i and its value into separate items and trims the historically space-prefixed driver-library option. Empty arguments and shell metacharacters survive the Unix protocol without evaluation; NUL is rejected. Cluster mode retains existing plugin archive/config file submission. The explicit commit default does not change the existing true batch behavior. Upstream source confirms Encoders.row and both 3.5 default methods (batch, streaming). This does not prove every reused Catalyst call binary-compatible.

1.2 Compatibility Impact

Fully compatible for existing launcher defaults; additive and bounded for Spark 3.5. Existing Spark 3.3/3.4 users retain their starter and scripts; the new artifact is opt-in. Legacy shell-text construction is preserved unless the new raw-argv property is set. No connector registration, old job configuration or checkpoint serialization is removed. Spark 3.5 is batch-first through the public starter, with partial adapter coverage rather than a claim of all-connector, all-deployment or checkpoint-restore compatibility. Windows arbitrary embedded quotes/metacharacters remain unsupported and documented.

1.3 Performance / Side-Effect Analysis

Reflection occurs when constructing the transform encoder, not for each row. There is one small launcher argv-file write/read and no new runtime background service. The extra starter increases distribution/test-reactor work; the explicit packaged verify step rebuilds its required upstream modules. No throughput claim is made. The Bash wrapper deliberately remains until Spark exits so its EXIT trap can clean the temporary file; replacing it with exec without moving cleanup would leak that file.

1.4 Error Handling and Logging

Reflective encoder invocation preserves RuntimeException/Error causes and reports missing/access failures with context. Unix and Windows reject Java failure/empty command results before Spark submission and preserve process status. NUL rejection stops the Java process so partially written argv is not submitted. Windows is not a general safe argument-serialization protocol; the documented restriction must remain. Spark lifecycle failures are still surfaced through existing command-execution handling.

Issue 1: Isolate JVM-global test properties and Spark sessions

Location: seatunnel-spark-starter-common/src/test/java/org/apache/seatunnel/core/starter/spark/SparkStarterTest.java:41-48; seatunnel-spark-3.5-starter/src/test/java/org/apache/seatunnel/core/starter/spark/execution/Spark35BatchWriteTest.java:90-140
Problem description: SparkStarterTest mutates shared system properties and clears them instead of restoring any prior value. The micro-batch test uses SparkSession.getOrCreate and then closes the returned session, so concurrent Spark tests could share/stop the same context. No enabled parallel execution or CI failure from these fixtures was established.
Potential risk: Conditional order/parallel-test contamination; the earlier unconditional Stable rating did not describe these shared-state boundaries.
Best improvement: Save/restore prior property values and use a test resource lock for their users. Isolate or serialize SparkContext ownership across Spark tests, keeping the exact row/commit assertions and bounded query cleanup.
Severity: Medium; non-blocking.
Raised by another reviewer: No

Issue 2: Refresh stale validation wording and add missing test-class contracts

Location: docs/en/getting-started/locally/quick-start-spark.md:30-33,111-114; matching docs/zh; PR body validation section; new Spark test classes
Problem description: The quick-start says dedicated micro-batch validation is still pending while the engine page correctly describes the implemented bounded sink-adapter test. The PR body names 28505d7 as current even though the reviewed head is 5184b37. Several new test classes, including Spark35LauncherTest, TestSpark35StarterJarContentIT and Spark35BatchWriteTest, lack class-level coverage/boundary comments.
Potential risk: Users and reviewers can mistake old evidence or a blanket no-microbatch statement for the current, narrower validation boundary.
Best improvement: Say public-starter streaming and checkpoint restore remain unvalidated, while the finite file-stream sink-adapter path is covered. Update exact-head CI/native-Windows facts and add short multiline test-class contracts. Do not turn the limited test into a general streaming-support claim.
Severity: Low; non-blocking.
Raised by another reviewer: No

Issue 3: Retain an explicit compatibility matrix for the reused translation layer

Location: seatunnel-core/seatunnel-spark-starter/seatunnel-spark-3.5-starter/pom.xml; TestContainerId.SPARK_3_5
Problem description: The starter intentionally reuses a translation layer compiled against 3.3, and normal PR connector CI excludes the Spark 3.5 container. Encoder construction and a bounded sink-adapter test cannot establish every connector/Catalyst/deployment path.
Potential risk: A path outside the exercised subset may fail at linkage or execution time despite green ordinary connector lanes.
Best improvement: Attach current-head explicitly selected or scheduled Spark 3.5 connector results, especially Hudi/Iceberg S3 and the intended deployment mode. Keep the documented limited support scope. A dedicated translation module or compile-compatibility check is a reasonable future boundary if additional API drift appears; no speculative framework rewrite is requested.
Severity: Low; non-blocking.
Raised by another reviewer: Continuation of the earlier Daniel/SEZ9 F3 compatibility discussion; not a new claim that a concrete linkage defect was found.

2. Code Quality Assessment

2.1 Coding Standards

The encoder class explains the runtime compatibility boundary; commit-coordinator and argv protocol changes have purpose comments. Existing modules and distribution entries remain. Spark 3.5.8 is an Apache release with Apache-2.0 metadata and provided Spark dependencies; S3 support versions are explicitly pinned in test staging. The official release and security page were consulted; no newly introduced Spark-specific advisory was established from those sources, which is not a full resolved-transitive vulnerability certification. The Apache image tag is versioned but not digest-locked. Small comment/document issues are in Issue 2.

2.2 Test Coverage and Test Stability

Test stability: Risk present. Issue 1 records conditional shared-state risk. Unix launcher tests use temporary directories, exact byte/argv assertions, bounded child waits and cleanup; the packaged test uses a real isolated JVM/classloader and captures spark-submit arguments without submitting a Spark job. Native-Windows packaged tests execute help, JVM failure and quoted config-parse boundaries, not successful Spark submission. The finite stream asserts termination, two input rows and exact committed values/INSERT kinds; its Windows skip is before resource creation because Hadoop native file support is unavailable. Tests do not establish public-starter streaming/recovery. Existing Hudi/Iceberg S3 fixtures are extended rather than replaced, but ordinary PR CI does not execute their Spark 3.5 container. No local compilation/tests/jobs were run.

Local analysis: fetched the head, created independent branch seatunnel-review-11307, read the complete 44-file diff and traced affected callers with git show/git grep. No local compilation, unit tests, integration tests, E2E, containers, or job execution was performed, as requested. These are source-analysis conclusions; execution evidence is the current-head CI below.

2.3 Documentation Updates

EN/ZH engine pages describe separate artifacts, retained 3.4 guidance, reused 3.3 translation, batch-only public starter, scheduled-only container and Windows limitations. Usage/Docker pages correctly require a matching Spark installation instead of implying a script rename upgrades an image. Issue 2 narrows stale quick-start/PR-body wording.

3. Architectural Soundness

3.1 Elegance of the Solution

Precise fix. The compatibility shim is correctly placed at the Spark encoder boundary, the default-method fix at the translation adapter, and argument serialization at the starter/launcher boundary. The smallest safe fix preserves old launchers while opting the new Unix launcher into raw argv. The global best evolution is a version-specific translation boundary if further binary differences appear; introducing it now without evidence would widen duplication and regression cost.

3.2 Maintainability

Keep the raw-argv and legacy-text paths explicit, and keep packaging tests isolated from the test JVM classpath. Reflective fallback must remain limited to missing methods; swallowing target invocation failures would hide real schema problems. Documented Windows restrictions and source-vs-runtime evidence remain material.

3.3 Extensibility

Other Spark versions can opt into a distinct artifact/property while preserving old defaults. Any expansion to public streaming or additional Catalyst APIs requires actual runtime coverage, not just a new version property or container registration.

3.4 Historical-Version Compatibility

No historical state format is changed and existing launcher defaults remain. Spark deployment/version migration still needs connector and runtime validation; the current bounded tests do not demonstrate restoration from an older Spark checkpoint. No production source defect was established beyond the fixed compatibility issues.

4. Issue Summary

Number	Issue	Location	Severity
1	Isolate JVM-global test properties and Spark sessions	`seatunnel-spark-starter-common/src/test/java/org/apache/seatunnel/core/starter/spark/SparkStarterTest.java:41-48; seatunnel-spark-3.5-starter/src/test/java/org/apache/seatunnel/core/starter/spark/execution/Spark35BatchWriteTest.java:90-140`	Medium
2	Refresh stale validation wording and add missing test-class contracts	`docs/en/getting-started/locally/quick-start-spark.md:30-33,111-114; matching docs/zh; PR body validation section; new Spark test classes`	Low
3	Retain an explicit compatibility matrix for the reused translation layer	`seatunnel-core/seatunnel-spark-starter/seatunnel-spark-3.5-starter/pom.xml; TestContainerId.SPARK_3_5`	Low

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Blockers — must be fixed
- No source blocker identified. Resolve current-head CI and the justified upstream synchronization described below, then obtain effective maintainer approval and the intended Spark 3.5 runtime evidence. The numbered improvements are non-blocking source follow-ups.
Recommended fixes — non-blocking
- Issue 1: Isolate JVM-global test properties and Spark sessions; Issue 2: Refresh stale validation wording and add missing test-class contracts; Issue 3: Retain an explicit compatibility matrix for the reused translation layer

Overall assessment: The earlier concrete launcher and logging problems are fixed. The appropriate next action is targeted CI/upstream reconciliation, not speculative launcher changes. Historical approval and author-reported pre-merge local results are not current-head acceptance.

Existing-review reconciliation: +1 to SEZ9 F1/F5: .cmd lines 53-64 capture/propagate Java status and lines 68-80 reject empty output then invoke Spark. F2/F6: .sh lines 73-91 use quoted Java argv and the argument array, with no eval. F4: parent shade exclusions remove embedded bindings; packaged isolated-classloader assertions require one external SLF4J 1.7 binding and no 2.x provider. F3: SparkRowEncoder plus the real bounded sink-adapter test and explicit engine-page limitations are the source evidence, not proof of the whole runtime. F7/F8: batch quick-start and retained 3.4 guidance are present; Issue 2 only corrects the stale breadth of the micro-batch statement. These addressed issues are not duplicated. The earlier recommendation to sync merely because the branch was behind is withdrawn; the new sync request is based on the concrete upstream fix below. Author replies have been incorporated into this single full review.

Head 5184b37259aad7f671cea4936dde13bec27cb92e. Live base dev at 3ac2d1cec6aa7b3741c4dacdd23020f778e23c02; comparison diverged, ahead 70 / behind 68; merge state BLOCKED. Behind/diverged alone is recorded metadata, not a request to synchronize upstream. Checks: Signal pull request update = SUCCESS; Build = FAILURE

Fork Build 41017512580 is complete and red. Both native-Windows unit lanes pass, including the packaged launcher step (JDK 17, JDK 11); the former Windows No-tests-executed gate is superseded. Current failures: Ubuntu unit JDK 17 Redis7Test.failedInitializationClosesConnections times out after five seconds waiting for client count equality; Redis code/test has no PR diff and no matching fetched-dev change. Engine JDK 11 has one BackpressureSlowSinkIT error waiting two minutes for a completed checkpoint. Unlike the earlier snapshot, dev now contains #12316, commit f867a54, which replaces unfair source-monitor reacquisition with explicit barrier/timer handoff and extends this same sustained-backpressure fixture. Git ancestry verifies the fix is absent from this head and present in fetched dev; this is a relevant upstream cure for the starvation scenario, not merely branch age. Please synchronize that upstream fix and rerun the affected current-head gates; it does not prove the separate Redis failure cured. Connectors part 3 JDK 11 was cancelled; cancellation does not count as green. Preserve diagnostics, retry only eligible remaining failures after checking active attempts, and do not modify Spark code to hide unrelated Redis/Zeta symptoms. No rerun or source synchronization was performed here.

Daniel has READ repository permission. This COMMENT records the technical conclusion; it is not an effective maintainer approval or a dismissal of any historical requested-changes entry. A write-capable maintainer owns the final repository approval gate.

SEZ9 · 2026-10-31T04:10:22Z

Thanks for the update — the latest review pass over the full diff at 5184b37 reports that the earlier F1–F8 launcher/logging/documentation items are now addressed in the source, and that the native-Windows unit lanes and their packaged-launcher step pass. A few things I'd like to pin down on those points before I sign off:

F2 / F6 (shell injection, unquoted $@/${args}/${CLASS_PATH}) — the move to having the JVM emit raw NUL-delimited arguments that Bash passes through as a quoted array is the right fix. Please confirm the eval path is fully gone from start-seatunnel-spark-3.5-connector-v2.sh (not just bypassed when the args-file property is set), and that ${CLASS_PATH} is quoted on the plain-java invocation as well.
F1 / F5 (Windows launcher) — please describe briefly how the .cmd now expands !errorlevel!/!CMD! and propagates the starter's real exit code when it fails with no stdout. The passing packaged-launcher step is reassuring, but I want to be sure the failure branch (non-zero exit, empty output) is exercised and does not exit 0.
F4 (dual SLF4J bindings) — which of log4j-slf4j-impl / log4j-slf4j2-impl ends up in starter/logging/* for the 3.5 starter? Only one SLF4J binding should be on the classpath of each launcher.
F3 (reusing the 3.3 translation jar on 3.5.8) — the explicit useCommitCoordinator() override and the runtime Encoders.row lookup in SparkRowEncoder cover the two known incompatibilities. Please note in the PR description that this is the intended approach (translation layer shared with 3.3, 3.5-specific differences handled at runtime), so the limitation is explicit.
F7 (streaming coverage) — the docs point Spark 3.5 users at the streaming template; is there now a micro-batch streaming test running against 3.5.8, or should the docs be adjusted to describe batch as the verified path?
F8 (Spark 3.4.x guidance) — please confirm docs/en/engines/spark.md now states what 3.4.x users should run.

Short pointers to the relevant hunks are enough; no need for a long write-up. Once those are confirmed I'm happy with the F1–F8 scope.

SEZ9 · 2026-11-01T04:24:06Z

Thanks for the update at 5184b37. From the thread I can see the Unix launcher now passes raw NUL-delimited arguments as a quoted argv array (F2/F6), SparkRowEncoder resolves Encoders.row at runtime with a fallback and useCommitCoordinator is explicit (F3), and a scheduled Spark 3.5 connector container is registered. I'll verify those against the diff rather than ask you to restate them.

A few points I can't yet trace from the thread:

F1 / F5 – How does the Windows launcher now handle !errorlevel!/!CMD! expansion relative to setlocal, and how is a non-zero exit code propagated when the starter java process fails with no stdout?
F4 – Which single SLF4J provider ends up under starter/logging/* for the Spark 3.5 starter?
F7 – Does the scheduled Spark 3.5 container exercise the micro-batch streaming path that the quick-start docs point Spark 3.5 users at? If not, softening that wording would be fine.
F8 – What guidance do Spark 3.4.x users now get in the engine docs?

Pointers to the relevant files in the diff are enough; I'll confirm from there.

DanielLeens · 2026-11-01T11:44:48Z

Thanks @SEZ9. Checked against the unchanged head:

F1/F5: the 3.5 .cmd uses setlocal disabledelayedexpansion at line 17; it does not use !CMD!/!errorlevel!. Java runs directly at :54, its status is saved on the next top-level line, and :61-64 returns that saved nonzero code even with empty stdout. TestSpark35StarterJarContentIT exercises real native-Windows help/failure/config paths; source string assertions alone are not the execution proof.
F2/F6: the 3.5 .sh has no eval fallback. It quotes CLASS_PATH, receives raw NUL-separated arguments and invokes the quoted array. The older 2.x/3.3 scripts are distinct, unchanged launchers.
F4: seatunnel-core/pom.xml copies log4j-slf4j-impl for the starter JVM's SLF4J 1.7 API. The Spark parent shade exclusions remove both bindings from the starter jar; the separate Spark-submit JVM uses Spark's own logging distribution. The 3.5 dependency exclusions do not select a second provider for starter/logging.
F3: the 3.5 starter explicitly depends on the 3.3 translation artifact; runtime encoder lookup and explicit commit-coordinator behavior are the intended adaptation, not blanket compatibility certification.
F7/F8: docs/en/engines/spark.md:19-30 and quick-start :105-115 retain the existing starter for Spark 3.4 and recommend batch for 3.5. A bounded micro-batch adapter test exists (Spark35BatchWriteTest:73), but the docs correctly do not claim full 3.5 streaming-template validation.

CI is still red in the previously identified Ubuntu Redis JDK17, engine JDK11 backpressure and cancelled connector-part-3 lane. The relevant backpressure fix #12316 is in latest dev and absent from this head; synchronize that fix before rerunning those gates, then investigate Redis separately if it repeats. This is a targeted CI recommendation with an identified upstream fix, not a branch-age-only request. No new source finding is added.

Checked 2026-11-01T11:44:42.189441+00:00; unchanged head 5184b37259aad7f671cea4936dde13bec27cb92e, base dev; compare diverged, ahead 70 / behind 77; merge blocked / BLOCKED; review decision REVIEW_REQUIRED. Behind/diverged is recorded independently and does not itself request a sync. Daniel has READ repository permission.

Signed-off-by: Goutam Adwant <workwithgoutam@gmail.com>

DanielLeens

Thanks for the quick turnaround on the last round. I re-reviewed the current head 47ae1be3d499 against my previous round at 5184b37259aa. The doc wording and test-class comment points are fixed, and dev (including the checkpoint-lock fix I pointed to) is now merged in, so the upstream-sync request is satisfied. The two shared-state test points and the stale PR-body head reference remain, all non-blocking, and the current-head CI is still pending.

What Problem Does This PR Solve?

User pain point: The Spark 3.3 starter hits removed RowEncoder APIs and a conflicting DataSource V2 default method when used with Spark 3.5.

Fix approach: Add an opt-in Spark 3.5.8 starter and scripts, resolve row encoding at runtime, explicitly implement the shared commit-coordinator default, package the new artifact alongside existing starters and register a scheduled Spark 3.5 connector container.

One-sentence summary and example: A Spark 3.5 batch user selects start-seatunnel-spark-3.5-connector-v2.sh; the JVM writes raw NUL-delimited arguments and Bash passes them as an array to the matching Spark installation.

1. Code Change Review

1.1 Core Logic Analysis

I diffed the PR-owned files between my previous head and this one. Production code, launchers, packaging descriptors and the container registration are unchanged. What changed:

docs/en and docs/zh quick-start: the stale "micro-batch validation is still pending" wording now says the bounded micro-batch test covers the row encoder and the streaming sink adapter, while streaming through the public starter and checkpoint restore remain unvalidated. This matches what Spark35BatchWriteTest actually exercises and what the engine page already states.
Docker pages (en/zh) and pom.xml (the spark.3.5.8.version property): these differ from my previous head only because of dev changes; against current dev the PR adds just the 3.5 note and the version property.
One-line class-level Javadoc was added to Spark35LauncherTest, TestSpark35StarterJarContentIT, Spark35BatchWriteTest, Spark35RowEncoderTest, SparkStarterTest and SparkRowEncoderTest, stating what each covers (and, for the batch-write test, what it does not).
The dev merge brings in #12316, which was the upstream fix for the BackpressureSlowSinkIT starvation failure from last round; I verified it is now an ancestor of this head.

Before:

ExpressionEncoder<Row> encoder = RowEncoder.apply(schema);
// 3.3-compiled BatchWrite + StreamingWrite inherits the older API shape.

After:

Encoder<Row> encoder = SparkRowEncoder.create(schema);
@Override public boolean useCommitCoordinator() { return true; }
// Unix launcher reads raw NUL-delimited items and invokes a quoted argv array.

Key Findings

Unix launcher: no eval, quoted argv array, NUL-delimited temp file removed by an EXIT trap, explicit handling of nonzero Java status and empty output.
Windows launcher captures the Java status before FOR /F, rejects empty command output and propagates the spark-submit status; the embedded-quote limitation stays documented.
SparkRowEncoder falls back only when Encoders.row is genuinely absent and preserves the original failure cause; useCommitCoordinator() returns true, preserving the old batch default.
Both distribution descriptors include the new scripts and starter; the 3.3/3.4 starters and scripts are untouched.

Complete runtime path

Unix/Windows launcher -> Java SparkStarter -> parse CLI and job env
 -> client/cluster-specific flags -> discover plugin jars -> buildFinal
 -> Unix UTF-8 NUL argv / Windows legacy command text -> spark-submit
 -> SeaTunnelSpark -> SourceExecuteProcessor batch Dataset
 -> TransformExecuteProcessor -> runtime SparkRowEncoder -> transformed Dataset
 -> SinkExecuteProcessor -> SparkSinkInjector -> SeaTunnelBatchWrite
 -> per-partition writer -> commit messages -> aggregated committer

1.2 Compatibility Impact

Fully compatible for existing launcher defaults; additive and bounded for Spark 3.5. The new artifact is opt-in, the legacy shell-text construction is preserved unless the raw-argv property is set, and no job configuration, connector registration or checkpoint format is removed. Spark 3.5 is batch-first through the public starter.

1.3 Performance / Side-Effect Analysis

Unchanged: reflection happens once at transform-encoder construction (not per row), one small argv-file write/read in the launcher, no new background service. The extra starter adds distribution and reactor work.

1.4 Error Handling and Logging

Unchanged and sound: reflective invocation preserves the target cause, Unix and Windows reject Java failure and empty results before Spark submission, and NUL in an argument aborts instead of submitting a partial argv.

Issue 1: Isolate JVM-global test properties and Spark sessions

Location: seatunnel-core/seatunnel-spark-starter/seatunnel-spark-starter-common/src/test/java/org/apache/seatunnel/core/starter/spark/SparkStarterTest.java:41-46; seatunnel-core/seatunnel-spark-starter/seatunnel-spark-3.5-starter/src/test/java/org/apache/seatunnel/core/starter/spark/execution/Spark35BatchWriteTest.java:94-147
Problem description: SparkStarterTest clears three shared system properties in @AfterEach rather than restoring whatever value was there before. Spark35BatchWriteTest obtains a session with SparkSession.builder()...getOrCreate() and closes it in a finally block, which stops a context that another Spark test in the same JVM may be sharing.
Potential risk: Conditional order or parallel-execution contamination between tests. I did not find an enabled parallel setting and no CI failure is attributed to either fixture, so this is a latent isolation risk.
Best improvement: Save and restore the prior property values (or use JUnit's @SetSystemProperty-style restore) and guard property users with a resource lock; give the micro-batch test its own session ownership or serialize Spark tests. Keep the exact-row and commit assertions.
Severity: Medium; non-blocking.
Raised by another reviewer: No (carried over from my previous round; unchanged on this head).

Issue 2: PR description still names an old head as current

Location: PR body, "Pending validation" section (references 28505d7724a9).
Problem description: The description says the current head is 28505d77 and that the Build for it was queued. The head is now 47ae1be3d499. The en/zh quick-start wording and the test-class comments from my previous Issue 2 are fixed; only this evidence reference is stale.
Potential risk: Reviewers may take outdated evidence as current-head validation.
Best improvement: Update the head SHA and the CI/native-Windows facts once the current Build finishes.
Severity: Low; non-blocking.
Raised by another reviewer: No.

Issue 3: Retain an explicit compatibility matrix for the reused translation layer

Location: seatunnel-core/seatunnel-spark-starter/seatunnel-spark-3.5-starter/pom.xml; TestContainerId.SPARK_3_5
Problem description: The 3.5 starter reuses a translation layer compiled against 3.3, and normal PR connector CI excludes the Spark 3.5 container. Encoder construction and the bounded sink-adapter test cannot prove every connector and Catalyst path.
Potential risk: A path outside the exercised subset could fail at linkage or execution time while ordinary lanes stay green.
Best improvement: Attach current-head results from an explicitly selected or scheduled Spark 3.5 connector run (Hudi/Iceberg S3 and the intended deployment mode in particular) and keep the documented support scope. A dedicated translation module is a reasonable future boundary only if more API drift shows up.
Severity: Low; non-blocking.
Raised by another reviewer: Continuation of the earlier Daniel/SEZ9 F3 discussion; not a new defect claim.

2. Code Quality Assessment

2.1 Coding Standards

The new test classes now carry class-level contracts. The encoder class and the argv protocol keep their purpose comments. No wildcard imports or stray changes were introduced by the merge.

2.2 Test Coverage and Test Stability

Test stability: Risk present. Issue 1 describes the shared-state risk. The launcher tests use temp directories, exact byte/argv assertions and bounded child waits; the packaged-jar test runs in an isolated JVM/classloader and captures spark-submit arguments without submitting a job; the finite file-stream test asserts termination, two input rows and exact committed values and row kinds. Ordinary PR CI does not execute the Spark 3.5 connector container (see Issue 3).

Local analysis: fetched the head, diffed it against the previously reviewed commit and against dev, and traced the affected callers with git show/git grep. No local compilation, unit tests, integration tests, E2E, containers, or job execution was performed; execution evidence is the CI below.

2.3 Documentation Updates

EN/ZH engine pages, quick-start, usage and Docker pages describe the separate artifact, retained 3.4 guidance, the batch-only public starter, the scheduled-only container and the Windows limits. The quick-start validation wording is now consistent with the engine page in both languages.

3. Architectural Soundness

3.1 Elegance of the Solution

Precise fix. The compatibility shim sits at the Spark encoder boundary, the default-method fix at the translation adapter, and argument serialization at the starter/launcher boundary. A version-specific translation module is the better long-term evolution only if more drift appears; doing it now would widen duplication without evidence.

3.2 Maintainability

Keep the raw-argv and legacy-text paths explicit and the packaging tests isolated from the test JVM classpath. Reflective fallback must stay limited to the missing-method case.

3.3 Extensibility

Other Spark versions can follow the same opt-in artifact and property pattern. Expanding to public-starter streaming or more Catalyst APIs needs real runtime coverage first.

3.4 Historical-Version Compatibility

No historical state format changes and existing launcher defaults are preserved. Restoring from an older Spark checkpoint is not demonstrated by the current tests.

4. Issue Summary

Number	Issue	Location	Severity
1	Isolate JVM-global test properties and Spark sessions	`SparkStarterTest.java:41-46`; `Spark35BatchWriteTest.java:94-147`	Medium
2	PR description still names an old head as current	PR body, Pending validation	Low
3	Retain an explicit compatibility matrix for the reused translation layer	3.5 starter `pom.xml`; `TestContainerId.SPARK_3_5`	Low

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Blockers — must be fixed
- No source blocker. The remaining gate is a green Build on this head (still pending, see below), plus the required maintainer approval and, as agreed earlier, current-head Spark 3.5 runtime evidence.
Recommended fixes — non-blocking
- Issue 1: isolate shared properties and Spark sessions in the tests.
- Issue 2: refresh the head SHA and evidence in the PR description.
- Issue 3: attach current-head Spark 3.5 connector results.

Overall assessment: The launcher, encoder and commit-coordinator work is sound on source inspection, and the previous docs and comment requests are addressed.

Existing-review reconciliation: my earlier Issue 2 is split above (the wording and test-class comment halves are done, the PR-body half remains), and my earlier request to synchronize with dev is satisfied by this head. SEZ9's F1 to F8 points were confirmed addressed last round and are not repeated; SEZ9's two most recent requests (comments of Oct 31 and Nov 1) ask for pointers to the same launcher, logging and docs hunks, which my previous review already mapped.

Head 47ae1be3d4997bd63ad9daffa7f7a8032c5df03a. Base dev ahead 71 / behind 0, so there is no upstream-sync request. Checks: Signal pull request update = SUCCESS; Build = in progress on the apache side, which only points to the fork. The fork Build run 41352875381 was still queued when I looked, so there is no execution result for this head yet. Last round's failures were Redis7Test.failedInitializationClosesConnections (no Redis change in this PR, and nothing in dev that would cure it, so it needs a fresh result rather than an assumption), BackpressureSlowSinkIT (the upstream fix is now merged in), and a cancelled connectors part. If any of these recur, retry only the failed job after checking for a newer active run; do not change Spark code to hide unrelated symptoms.

Daniel has READ repository permission. This COMMENT records the technical conclusion; it is not an effective maintainer approval or a dismissal of any historical requested-changes entry. A write-capable maintainer owns the final repository approval gate.

DanielLeens

Follow-up on the exact head 47ae1be3d4997bd63ad9daffa7f7a8032c5df03a (unchanged since my last review).

The CI fact I cited before has changed: I previously noted that CI was still pending for this head. The apache-side Build check is now completed with SUCCESS on this exact head, and Signal pull request update is also SUCCESS. The branch is not behind dev.

Source-side blockers from my side: none. My last review found no source blocker; the remaining items (shared test-property isolation, the stale head reference in the PR description, and attaching Spark 3.5 runtime evidence) are non-blocking.

One item still prevents me from approving: reviewer SEZ9 has a CHANGES_REQUESTED review recorded on an older head. I confirmed last round that their F1 to F8 points are addressed, but that entry is still open on the PR and I do not want to override it. SEZ9, could you re-check the current head and update or dismiss your review if you agree? Once that is cleared there is no remaining blocker on my side, and a write-capable maintainer would need to give the effective approval and merge, because this account has READ permission only.

SEZ9 · 2026-11-03T02:33:56Z

Thanks for the update on head 47ae1be3d4997bd63ad9daffa7f7a8032c5df03a.

Before I clear my earlier change request, could you briefly confirm how each of the previous points is handled on this head? I don't see that spelled out in the thread yet:

F1 / F5 (Windows .cmd launcher): !errorlevel!/!CMD! are now expanded while delayed expansion is still enabled (so the call target is the real command rather than the literal !CMD!), and a failing starter java process with no stdout now propagates a non-zero exit code instead of exiting 0.
F2 / F6 (shell launcher): whether the eval of the config-assembled command line was changed or hardened, and that args=$@, ${args} and ${CLASS_PATH} are now quoted so argument boundaries and globs are preserved.
F3 (shaded seatunnel-translation-spark-3.3): the approach taken — a Catalyst compatibility check against 3.5.8 at build/test time, or a short note on why runtime detection is acceptable here.
F4 (logging jars): that only one SLF4J binding ends up under starter/logging/* for the Spark 3.5 starter, or how the log4j-slf4j-impl / log4j-slf4j2-impl overlap is avoided.
F7 (tests): whether a streaming (micro-batch) test was added for Spark 3.5, or a run of the streaming template on Spark 3.5.8 as evidence.
F8 (docs): that Spark 3.4.x guidance was added alongside the "Spark 3.3.x" wording for seatunnel-spark-3-starter.jar.

A short per-item note or pointers to the relevant changes is enough. Once confirmed, I'll update my review so the PR can move forward.

Signed-off-by: Goutam Adwant <workwithgoutam@gmail.com>

goutamadwant · 2026-11-03T04:57:59Z

@davidzollo @DanielLeens Fixed the remaining test-isolation issues: JVM properties are restored correctly, and the micro-batch fixture no longer borrows or stops another test’s Spark context. The PR body now reflects the current evidence and limitations.

Could you review and merge once fresh CI passes and the outstanding review gate is cleared? Thanks!

DanielLeens

Thanks for closing out the last open items so quickly, @goutamadwant. I re-reviewed the current head 1822620f0982 against my previous round at 47ae1be3d499. The only change is one test-only commit touching two files; production code, launchers, packaging descriptors, docs and the container registration are byte-identical to the head I cleared last time. My previous Issue 1 (test isolation) and Issue 2 (stale head in the PR body) are both fixed. No new source finding.

What Problem Does This PR Solve?

User pain point: the Spark 3.3 starter hits removed RowEncoder APIs and a conflicting DataSource V2 default method on Spark 3.5.

Fix approach: an opt-in Spark 3.5.8 starter, launchers and distribution artifact; runtime row-encoder selection; explicit useCommitCoordinator(); a scheduled Spark 3.5 connector container.

One-sentence summary: a Spark 3.5 batch user picks start-seatunnel-spark-3.5-connector-v2.sh/.cmd, and the existing 3.3/3.4 paths are untouched.

1. Code Change Review

1.1 Core Logic Analysis

Delta since my last review (git diff 47ae1be3..1822620f): SparkStarterTest.java and Spark35BatchWriteTest.java only.

seatunnel-core/seatunnel-spark-starter/seatunnel-spark-starter-common/src/test/java/org/apache/seatunnel/core/starter/spark/SparkStarterTest.java:42-73: the class now holds @ResourceLock(Resources.SYSTEM_PROPERTIES); @BeforeEach saves the three starter properties and clears them, @AfterEach restores the saved value (or clears it when it was absent). This is exactly the save-and-restore plus resource lock I asked for.
seatunnel-core/seatunnel-spark-starter/seatunnel-spark-3.5-starter/src/test/java/org/apache/seatunnel/core/starter/spark/execution/Spark35BatchWriteTest.java:69-70: @Isolated + @Execution(SAME_THREAD), so no other test runs concurrently with the Spark fixture.
Spark35BatchWriteTest.java:114-117: the micro-batch fixture now refuses to run when a SparkContext is already active, instead of borrowing it via getOrCreate() and stopping it in finally. Spark35BatchWriteTest.java:84-106 proves that an existing context is neither borrowed nor stopped.
The exact-row, termination and numInputRows assertions in boundedMicroBatchCommitsRowsThroughStreamingAdapter are unchanged, so no existing test was weakened.

if (SparkContext$.MODULE$.getActive().isDefined()) {
    throw new IllegalStateException("The micro-batch fixture requires its own SparkContext");
}

The runtime path I traced last round (launcher -> SparkStarter -> spark-submit -> SeaTunnelSpark -> SparkRowEncoder -> SparkSinkInjector -> SeaTunnelBatchWrite -> committer) is unchanged on this head.

1.2 Compatibility Impact

Fully compatible, unchanged from my previous review: the new artifact is opt-in, the legacy launcher text path is preserved, and no option, default, registration or state format changes.

1.3 Performance / Side-Effect Analysis

None from this delta (test-only).

1.4 Error Handling and Logging

Unchanged. The new fixture guard fails loudly with a clear message instead of silently sharing a context, which is the right behavior for a test.

Issue 1 (nit): Fixture self-test adds little regression value

Location: SparkStarterTest.java:76-96 (cleanupRestoresAbsentEmptyAndNonemptyProperties)
Problem description: This test exercises the test class's own @BeforeEach/@AfterEach helpers rather than any production behavior. It is correct (the outer @AfterEach still restores the real originals afterward), but it guards test plumbing, not SparkStarter.
Potential risk: None for production; slight maintenance overhead.
Best improvement: Optional; fine to keep or drop.
Severity: Low; non-blocking.
Raised by another reviewer: No.

2. Code Quality Assessment

2.1 Coding Standards

Imports are explicit, class-level Javadoc on both test classes is retained, and the guard message is self-explanatory.

2.2 Test Coverage and Test Stability

Test stability: Stable for the touched fixtures. The previous latent risk (cleared instead of restored properties, and a getOrCreate() session closed under another test) is removed by SparkStarterTest.java:42-73 and Spark35BatchWriteTest.java:69-70,114-117. The new microBatchDoesNotBorrowOrStopAnExistingContext asserts the concrete outcome (existing context not stopped and still able to run range(1).count()), so it would fail if the guard were removed.

Ordinary PR CI still does not run the scheduled Spark 3.5 connector container (my earlier non-blocking Issue 3 stands: attaching a current-head Spark 3.5 connector run is still welcome, but it is not a merge blocker).

Local analysis: diffed the new head against the previously reviewed head and read both changed test files at the head commit. No local compilation, tests, containers or jobs were run.

2.3 Documentation Updates

No doc change in this delta. The PR description now reflects the current evidence and limitations, which closes my previous Issue 2.

3. Architectural Soundness

Unchanged: the encoder shim, the default-method fix and the argv protocol each sit at the narrowest boundary. A dedicated 3.5 translation module remains a future option only if more API drift appears.

4. Issue Summary

Number	Issue	Location	Severity
1	Fixture self-test adds little regression value (nit)	`SparkStarterTest.java:76-96`	Low
(prev 3)	Attach a current-head Spark 3.5 connector run	3.5 starter `pom.xml`; `TestContainerId.SPARK_3_5`	Low

Previous-round status: Issue 1 (isolate properties and Spark sessions) fixed; Issue 2 (stale head in PR body) fixed; Issue 3 still open, non-blocking.

5. Merge Recommendation

Conclusion: Ready to merge

Blockers - must be fixed
- No source blocker. The only remaining gate is CI on this head: Build is still pending on the apache side, and the real run in the fork (Build 41580340332) is in progress, so there is no execution result yet. Signal pull request update = SUCCESS.
Recommended - non-blocking
- The nit above and attaching a Spark 3.5 connector run.

@SEZ9, on your Nov 3 question: the launchers, POMs and docs are unchanged between 47ae1be3 and this head, so the per-item pointers in my Nov 1 reply (.cmd setlocal disabledelayedexpansion at line 17 and exit-code propagation at :61-64; .sh quoted array with no eval; single SLF4J 1.7 binding asserted in TestSpark35StarterJarContentIT.java:75-88; batch guidance for 3.5 and retained 3.4 guidance in docs/en/engines/spark.md:19-30; bounded micro-batch test in Spark35BatchWriteTest) still apply line-for-line. If you agree, updating your CHANGES_REQUESTED entry would remove the last non-CI gate.

Head 1822620f09823e2d7762334d260e110c2583f977. Base dev: ahead 72 / behind 5, merge state BLOCKED (review required); behind-by is small and not a sync request. I am leaving this as a COMMENT because CI is still running; once the Build is green there is nothing further from my side. This account has READ permission, so a write-capable maintainer owns the effective approval and merge.

DanielLeens

Quick CI follow-up on the unchanged head 1822620f09823e2d7762334d260e110c2583f977, since my review above said the fork Build was still running.

The fork Build 41580340332 has now finished with one failed job: unit-test (17, ubuntu-latest). The single error (1 of 695 tests in seatunnel-engine-server) is PipelineCleanupRecordHazelcastSerializationTest.testPutAndGetAcrossMembers, where Hazelcast could not start because Port [36273] is already in use and auto-increment is disabled (java.net.BindException: Address already in use). That is a local port collision on the runner in an engine-server test. This PR does not touch seatunnel-engine-server, and the same test passed in the other unit-test lanes of this run, so I consider it unrelated to the Spark 3.5 change. Latest dev has no newer change to that test, so syncing would not change anything.

There is no newer run on this head. Please rerun only the failed job; no code change is needed.

My source conclusion is unchanged: no source blocker from my side. The remaining gates are a green Build on this head, SEZ9's open CHANGES_REQUESTED entry, and a write-capable maintainer's approval. Thanks!

SEZ9 · 2026-11-04T02:54:16Z

Follow-up on the unchanged head 1822620f09823e2d7762334d260e110c2583f977.

The fork Build 41580340332 has finished with a single failed job, 125838849297 (unit-test, Java 17, ubuntu-latest). The only failure is PipelineCleanupRecordHazelcastSerializationTest.testPutAndGetAcrossMembers in seatunnel-engine-server, where Hazelcast could not start because Port [36273] is already in use and auto-increment is disabled (java.net.BindException: Address already in use). That is a port collision on the runner; this PR does not touch seatunnel-engine-server, and the same test passed in the other unit-test lanes of the run, so I consider it unrelated to the Spark 3.5 change.

Please rerun only the failed job — no code change is needed for it. My conclusion is unchanged: no source blocker from my side. The remaining gates are a green Build on this head, the open change request from the other reviewer, and a write-capable maintainer's approval. Thanks!

github-actions Bot added document core SeaTunnel core module Spark labels Jul 3, 2026

goutamadwant mentioned this pull request Jul 3, 2026

[Feature] Support SeaTunnel on Spark 3.5.8 #11128

Open

2 of 3 tasks

DanielLeens suggested changes Jul 3, 2026

View reviewed changes

goutamadwant force-pushed the GH-11128 branch from 825fd00 to 5355715 Compare July 3, 2026 14:09

DanielLeens reviewed Jul 4, 2026

View reviewed changes

goutamadwant added 2 commits July 3, 2026 23:30

[Feature][Spark] Support Spark 3.5.8

463cf8a

Signed-off-by: goutamadwant <workwithgoutam@gmail.com>

[Feature][E2E] Align Spark runtime with 3.5.8

73f43ab

Signed-off-by: goutamadwant <workwithgoutam@gmail.com>

goutamadwant force-pushed the GH-11128 branch from 8e31bcc to 73f43ab Compare July 4, 2026 06:39

github-actions Bot added the e2e label Jul 4, 2026

DanielLeens reviewed Jul 4, 2026

View reviewed changes

Merge branch 'dev' into apacheGH-11128

467c888

DanielLeens approved these changes Jul 5, 2026

View reviewed changes

github-actions Bot added the reviewed label Jul 5, 2026

Merge branch 'dev' into apacheGH-11128

115ecdc

DanielLeens approved these changes Jul 6, 2026

View reviewed changes

Merge branch 'dev' into apacheGH-11128

06dcac9

DanielLeens approved these changes Jul 7, 2026

View reviewed changes

Merge branch 'dev' into apacheGH-11128

b67cdc8

DanielLeens reviewed Jul 8, 2026

View reviewed changes

DanielLeens reviewed Jul 9, 2026

View reviewed changes

Merge branch 'dev' into apacheGH-11128

3780c41

SEZ9 suggested changes Jul 10, 2026

View reviewed changes

github-actions Bot removed the reviewed label Jul 10, 2026

DanielLeens suggested changes Jul 10, 2026

View reviewed changes

goutamadwant and others added 2 commits October 24, 2026 09:04

[Test][Spark] Validate micro-batch sink and packaged Unix launcher

d804587

Related to apache#11128 and apache#11307.

Merge branch 'dev' into apacheGH-11128

28505d7

DanielLeens reviewed Oct 25, 2026

View reviewed changes

[Test][Spark] Run packaged Spark 3.5 launcher checks on Unix

380764c

Signed-off-by: Goutam Adwant <workwithgoutam@gmail.com>

DanielLeens reviewed Oct 25, 2026

View reviewed changes

[Test][Core] Guard Spark file-stream test on Windows

4b80eba

Signed-off-by: Goutam Adwant <workwithgoutam@gmail.com>

DanielLeens reviewed Oct 28, 2026

View reviewed changes

SEPURI-SAI-KRISHNA mentioned this pull request Oct 28, 2026

[Fix][Connector-V2] Keep the column of a null field in the Druid sink #13046

Merged

5 tasks done

[Fix][CI] Isolate packaged Spark launcher integration tests

5184b37

Signed-off-by: Goutam Adwant <workwithgoutam@gmail.com>

DanielLeens reviewed Oct 31, 2026

View reviewed changes

[Docs][Spark] Clarify validation scope and merge dev checkpoint fix

47ae1be

Signed-off-by: Goutam Adwant <workwithgoutam@gmail.com>

DanielLeens reviewed Nov 2, 2026

View reviewed changes

DanielLeens reviewed Nov 3, 2026

View reviewed changes

[Test][Spark] Isolate starter properties and Spark contexts

1822620

Signed-off-by: Goutam Adwant <workwithgoutam@gmail.com>

DanielLeens reviewed Nov 3, 2026

View reviewed changes

Conversation

goutamadwant commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose of this pull request

Does this PR introduce any user-facing change?

How was this patch tested?

Regression evidence

Local follow-up validation

Hosted evidence and remaining limits

Check list

Uh oh!

DanielLeens left a comment

Choose a reason for hiding this comment

What this PR changes

Runtime chain I checked

Current-head findings

Merge conclusion

Conclusion: can merge after fixes

Uh oh!

goutamadwant commented Jul 3, 2026

Uh oh!

DanielLeens left a comment

Choose a reason for hiding this comment

What this PR solves

Runtime chain I checked

Findings

Merge conclusion

Conclusion: can merge after fixes

Uh oh!

DanielLeens left a comment

Choose a reason for hiding this comment

Uh oh!

goutamadwant commented Jul 4, 2026

Uh oh!

DanielLeens left a comment

Choose a reason for hiding this comment

Merge conclusion: can merge

Uh oh!

DanielLeens commented Jul 5, 2026

Uh oh!

DanielLeens left a comment

Choose a reason for hiding this comment

What this PR changes

Runtime path checked

Compatibility / side effects

Tests

Merge conclusion: can merge

Uh oh!

DanielLeens commented Jul 7, 2026

Uh oh!

DanielLeens left a comment

Choose a reason for hiding this comment

What this PR solves

Runtime path checked

Merge conclusion

Conclusion: can merge

Uh oh!

DanielLeens left a comment

Choose a reason for hiding this comment

What this PR fixes

Runtime / review path checked

Current review findings

Testing and local verification

Merge conclusion

Conclusion: can merge after CI

Uh oh!

DanielLeens left a comment

Choose a reason for hiding this comment

Uh oh!

SEZ9 left a comment

Choose a reason for hiding this comment

What this PR fixes

Runtime chain I rechecked

Findings

Review conclusion

Conclusion: can merge after the blocking items are fixed

Uh oh!

DanielLeens left a comment

Choose a reason for hiding this comment

What this PR fixes

goutamadwant commented Jul 3, 2026 •

edited

Loading

Issue 1 (Blocker): `Encoders.row()` is Spark 3.5+ only, breaks 3.3/3.4 runtime

Issue 3: Windows `.cmd` still hands a single command string to `spark-submit.cmd` (carryover)

Issue 5: `.sh` keeps a parent bash instead of `exec` (carryover)

Issue 6: Possible NPE instead of an assertion message if the slf4j manifest has no `Implementation-Version` (carryover nit)

What the CI says (head `380764cb`, unchanged since my review)

2. `SplitClusterFaultToleranceIT` is a known engine flake