DanielLeens commented on PR #11307:
URL: https://github.com/apache/seatunnel/pull/11307#issuecomment-5662011492
Thanks for the detailed status check, @SEZ9. I re-read every one of F1-F8
directly against the current head (`2b832dc0e826f4fd456946f525ade7d83ddb4bbd`)
rather than going off my own prior summary, and I can point you to exact lines
for each:
- **F1 (`.cmd` `disabledelayedexpansion`/`!CMD!` ordering)** — resolved, and
resolved differently than the original bug shape. The current script sets
`setlocal disabledelayedexpansion` once at the very top
(`start-seatunnel-spark-3.5-connector-v2.cmd:17`) and never uses `!var!`
delayed-expansion syntax anywhere in the file — it's `%var%` throughout,
including the `CMD` variable set via the `for /f ... usebackq` loop (`:66-67`)
and consumed at `:77`. There's no ordering hazard left because there's no
delayed-expansion path left to order.
- **F2 (`eval` in the `.sh`)** — resolved. The old `CMD=$(java ...) && eval
$(echo "${CMD}" | tail -n 1)` pattern is gone entirely. The new script writes
NUL-delimited args to a temp file only when a system property is set, reads
them back into a real bash array
(`start-seatunnel-spark-3.5-connector-v2.sh:81-84`), and execs
`"${SPARK_HOME}/bin/spark-submit" "${spark_args[@]}"` directly (`:90`) — no
`eval` anywhere in the file. `Spark35LauncherTest` actually runs the script
with a payload containing `$(touch marker)`/backticks/`$HOME`/`*` and asserts
nothing gets re-evaluated, so this is behaviorally proven, not just
structurally absent.
- **F5 (`.cmd` swallowing the java exit code)** — resolved. `EXIT_CODE` is
captured immediately after the java invocation (`:52-53`) and propagated via
`exit /b %EXIT_CODE%` on the non-234/non-zero branch (`:59-62`); the `for /f`
loop that used to eat the real exit status is gone from that path entirely.
- **F6 (unquoted `args`/`CLASS_PATH` in the `.sh`)** — resolved. `java_opts`
is a proper bash array built and appended to element-by-element (`:53-63`),
`CLASS_PATH` is expanded quoted at the `-cp` call site (`:74`), positional args
are passed as `"$@"` (`:74`), and `spark_args` elements are read
one-per-NUL-terminator into an array and expanded as `"${spark_args[@]}"`
(`:90`) — no word-splitting/globbing surface left. (One caveat, noted below in
the F4 answer: `java_opts=(${JAVA_OPTS:-})` at `:53` is deliberately left
unquoted with `set -f`/`set +f` bracketing it, per the comment at `:50`, to
preserve the historical whitespace-separated `JAVA_OPTS` behavior without
pathname expansion — that's intentional back-compat, not a miss.)
- **F3 (reflection shim as a known limitation)** — resolved via docs, as
intended. `docs/en/engines/spark.md` and the zh counterpart both state the 3.5
starter "reuses the translation layer compiled against Spark 3.3" and that
row-encoder/batch-write tests exercise selected paths on 3.5.8 without full
coverage — so the limitation is explicit, not implied.
- **F8 (3.4.x guidance)** — resolved. `docs/en/engines/spark.md:19-21`: "For
an existing Spark 3.4 deployment, retain the existing
`seatunnel-spark-3-starter.jar`" — directly answers this.
- **F4 (SLF4J binding)** — I want to be precise here rather than just say
"resolved": what I checked is `seatunnel-spark-starter/pom.xml`'s shade
`<artifactSet><excludes>`, which lists both
`org.apache.logging.log4j:log4j-slf4j-impl` and `log4j-slf4j2-impl` (lines
101-102) as **excluded from the shaded jar** (Spark's own server lib already
provides one of them) — so there's no double-binding *inside the fat jar
itself*. That's a real, correct answer to "are both bindings shaded in," but
it's not the same claim as "the packaged `starter/logging/` directory of a
rebuilt distribution contains only one binding" — I haven't personally looked
at that directory's contents from an actual `seatunnel-dist` build, which is
what I flagged as Issue 2 (Low) in my last review. So: F4 as "shade-exclude
correctness" — confirmed. F4 as "verified from a real built distribution" —
still open, tracked as my Issue 2, non-blocking.
- **F7 (streaming/micro-batch test)** — still open, tracked as my Issue 3
(Low). The docs no longer overclaim (they now point Spark 3.5 users at the
batch template instead of streaming), which reduces the risk from "undocumented
gap" to "coverage gap," but a dedicated streaming test doesn't exist yet.
So: F1/F2/F3/F5/F6/F8 are resolved and independently re-verified by me
against current source; F4 and F7 are open but Low-severity and already tracked
in my last review as non-blocking. I don't see a source-level blocker remaining.
On CI, since it's part of what I'm asked to factor in: as of this comment
the apache-side `Build` check is still `IN_PROGRESS` on this head — I don't
have a completed result to report yet either way, so I'm not drawing a
conclusion from it this round.
--
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]
