DanielLeens commented on PR #11307:
URL: https://github.com/apache/seatunnel/pull/11307#issuecomment-5645092837
@SEZ9 Thanks for the detailed list — I went through each item against the
actual current head (`169864a0be9`) rather than just tracking status, since the
previous summaries didn't call out F1-F8 individually. Here is my own
independent read of each:
**F1 (cmd, `disabledelayedexpansion` before `!errorlevel!`/`!CMD!`) and F5
(empty-stdout exit code swallowed)** — Not new to this PR, and not something
the 3.5.8 diff introduced. I pulled the shipped
`start-seatunnel-spark-3-connector-v2.cmd` (the existing, currently-released
Spark 3.3 launcher) and it has the *exact same* `for /f ... do (set "CMD=%%i" &
setlocal disabledelayedexpansion & if !errorlevel! ... )` block, byte-for-byte.
The same pattern is also in `seatunnel-spark-2-starter` and all three Flink
starters (1.3/1.5/2.0). The new `start-seatunnel-spark-3.5-connector-v2.cmd` is
a straight copy of that existing template with the jar name swapped, which is
exactly what we should expect for consistency with the sibling starters. On the
substance: this is the documented "disable delayed expansion mid-loop to output
a captured value safely" idiom — the `!var!` tokens inside the already-parsed
`for /f do (...)` compound block resolve against the delayed-expansion state
that
was active when the block was first read (under the outer `setlocal
enabledelayedexpansion`), not per-statement at execution time, so
`disabledelayedexpansion` here is stopping the *value* of `CMD` from being
re-expanded a second time (it may contain `%`/`&`/`^` from job config paths),
not breaking the `!CMD!`/`!errorlevel!` references themselves. If that
reasoning were wrong, every Windows user of every existing SeaTunnel starter
would have had a fully non-functional `call`/`exit` path for a long time, which
isn't consistent with this being long-shipped code. Same logic for F5 (a `java`
crash with empty stdout means the `for /f` body never executes at all, so no
exit-code propagation happens) — real edge case, but identical in the existing
Spark 3.3/Spark 2/Flink `.cmd` scripts today. I'd treat both as a pre-existing
Windows-launcher-family issue worth its own infra ticket if we want to fix it
everywhere, not something to hold this PR on.
**F2 (sh `eval`) / F6 (unquoted `args=$@`, `${CLASS_PATH}`)** — Same
conclusion: I diffed the new `start-seatunnel-spark-3.5-connector-v2.sh`
against the current `start-seatunnel-spark-3-connector-v2.sh` and the `eval
$(echo "${CMD}" | tail -n 1)` plus the unquoted `args=$@`/`${CLASS_PATH}` are
identical in both, inherited verbatim. Not a regression from this diff.
**F3 (translation-spark-3.3 shaded into the 3.5 starter — Catalyst binary
compat)** — I want to correct something from my own Aug 9 review here rather
than let it stand uncorrected: I'd claimed "zero hits" on
`org.apache.spark.sql.catalyst.*`/`org.apache.spark.sql.execution.*` outside
the encoder shim. Re-grepping the current `seatunnel-translation-spark-3.3`
module, that's not quite right — `org.apache.spark.sql.catalyst.InternalRow` is
imported in 5 files (`ParallelBatchPartitionReader`,
`SeaTunnelBatchPartitionReader`, `SeaTunnelBatchPartitionReaderFactory`,
`SeaTunnelMicroBatchPartitionReader`,
`SeaTunnelMicroBatchPartitionReaderFactory`). That said, I don't think it
changes the compatibility conclusion: `InternalRow` is Spark's DataSource V2
wire-format class (`PartitionReader<InternalRow>`), used identically by every
V2 connector (Iceberg, Delta, etc.) and stable across 3.0-3.5+ — it's a
different risk category from `RowEncoder`/`ExpressionEncoder`, which are the gen
uinely internal/unstable Catalyst APIs that actually broke between 3.3 and 3.5
(and which already got the `SparkRowEncoder` reflection fallback). Testing
backing the V2 read/write path specifically:
`SparkRowEncoderTest`/`Spark35RowEncoderTest`/`Spark35BatchWriteTest` per the
Aug 9 review, plus the Iceberg and Druid Spark E2E lanes (both confirmed green
in the Sept 9 clean run, `33731860742` attempt 5) which exercise the full
`InternalRow`-based partition-reader path end-to-end against a real 3.5.8
runtime — that's real behavioral coverage, not just "it compiles against 3.3."
**F4 (`log4j-slf4j-impl` + `log4j-slf4j2-impl` both under
`starter/logging/*`)** — Partially verified, and I want to be straight about
the part I can't close from source alone. Confirmed clean:
`seatunnel-spark-3.5-starter/pom.xml` excludes `log4j-slf4j2-impl` and
`jul-to-slf4j` from all three of its own Spark 3.5.8 dependencies
(`spark-streaming`/`spark-core`/`spark-sql`), and `seatunnel-dist/pom.xml`'s
shared provided-scope dependency set (used by all starters, including the
existing 3.3 one) declares only `log4j-slf4j-impl`. So the 3.5-starter's own
declared dependency graph is clean and consistent with the existing 3.3
starter's setup. What I can't rule out purely by reading poms is whether the
final assembled `starter/logging/*` directory in a built distribution actually
ends up with both bindings once the full reactor's transitive graph is resolved
— that needs an actual `mvn dependency:tree` on the 3.5-starter module (or
inspecting the packaged binary's `starter/logging
` folder), which is outside a static-review pass. @goutamadwant, could you run
`mvn dependency:tree -pl
seatunnel-core/seatunnel-spark-starter/seatunnel-spark-3.5-starter` (or just
`ls` the built dist's `starter/logging/` dir) and confirm only one SLF4J
binding shows up there?
**F7 (streaming docs vs. row-encoder/batch-write-only tests)** — Confirmed
as described: `quick-start-spark.md`'s Spark 3.5.x example runs
`v2.streaming.conf.template`, and the new test coverage for the 3.5 starter is
`SparkRowEncoderTest`, `Spark35RowEncoderTest`, `Spark35BatchWriteTest`,
`SparkStarterTest`, `Spark35LauncherTest` — no micro-batch/streaming-mode test
specific to the new starter. Fair, correctly framed as non-blocking.
**F8 (3.4.x left out of the starter guidance)** — Confirmed verbatim in
`docs/en/engines/spark.md`: "`seatunnel-spark-3-starter.jar` for Spark 3.3.x" /
"`seatunnel-spark-3.5-starter.jar` for Spark 3.5.x", with no mention of 3.4.x
anywhere in that section. Small, easy doc fix.
**Bottom line from my side**: F1/F2/F5/F6 are pre-existing patterns
inherited unchanged from the already-shipped Spark 3.3/Spark 2/Flink launcher
scripts, not regressions in this diff — I wouldn't block on those here. F3
holds up with corrected, more precise evidence. F4's pom-level hygiene checks
out but needs the author's `dependency:tree`/packaged-dist confirmation to
fully close. F7/F8 are real, low-effort doc/test gaps worth a quick fix before
merge but not structural blockers. Once F4 gets a concrete answer and F8 gets a
one-line doc fix, I don't see anything left that should stop an approving
review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
