DanielLeens commented on PR #11307:
URL: https://github.com/apache/seatunnel/pull/11307#issuecomment-5713093833
@goutamadwant @SEZ9 - thanks both. No new commit since `d3d34f292` (still
the exact head), so this is a reply, not a fresh full review.
**FilterRowKind test - confirmed unrelated to this PR.** Good, this closes
the one thing I was waiting on from my last review:
`testFilterRowKindMultiTable` failed on Flink 1.15.3/1.18.0 containers, not
`Spark35Container`, so `transform-v2-it-part-1` in run `34918482860` is not a
regression from this diff. That leaves `engine-v2-it` and `all-connectors-it-2`
as the other two already-confirmed-unrelated failure categories from that same
run.
**On the Windows launcher (SEZ9's item 1):** I re-checked the actual
current-head file just now rather than relying on my own earlier notes, and the
specific bug you describe - delayed expansion comparing literal strings, and a
loop swallowing the real exit code on empty stdout - doesn't match what's on
`d3d34f292` today. `start-seatunnel-spark-3.5-connector-v2.cmd:51-53` runs
`java` directly (not inside a `FOR /F`) and captures `%errorlevel%` immediately
afterward, with the line-51 comment explicitly noting this was done to avoid
the "FOR /F doesn't preserve child exit status" trap; `setlocal
disabledelayedexpansion` (line 17) is safe here because nothing later reads a
variable via `!var!` inside the same block. Checking the file's commit history:
this shape was introduced by `ee21221df89c` ("[Fix][Core] Preserve Spark 3.5
launcher arguments and exit codes", 2026-09-13T04:07:28Z), which landed after
my 00:50 review that same day - so the version I originally reviewed did have
the
bug you're describing, it has just been fixed since. What's genuinely still
open (my own "Issue 1" from the Sept-15 review, a narrower point than the
expansion bug) is that the resolved args are still assembled into one string
and invoked via `call "%SPARK_HOME%\bin\spark-submit.cmd" %CMD%` (line 77)
rather than an argv array - same injection class already closed on the `.sh`
side via the NUL-delimited args file, still open on `.cmd` since `cmd.exe` has
no native argv-array equivalent. A real run on a Windows box, as you're asking
for, is still the right way to close this out - static reading alone doesn't
prove `spark-submit.cmd` parses `%CMD%` the way we expect.
**F3 (3.3 translation-layer reuse) and F8 (Spark 3.4.x guidance):** both
already resolved via documentation - `docs/en/engines/spark.md` (~lines 19,
23-27) and the `zh` mirror state this explicitly, unchanged since my last full
review. No action needed there unless something in those docs reads wrong to
either of you.
**F4 (dual SLF4J bindings) and F7 (micro-batch streaming coverage):** still
open, still Low/non-blocking per my last assessment - F4 needs a look at an
actual built `seatunnel-dist` rather than just the pom excludes, F7 needs a
dedicated streaming test under `seatunnel-spark-3.5-starter/src/test/**` (or
the manual-run-plus-pasted-output SEZ9 suggests as a stopgap). Both reasonable
to fold into whatever commit addresses the Windows launcher.
I'll hold off on a fresh full review until the next commit lands.
--
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]
