SEZ9 commented on PR #11307:
URL: https://github.com/apache/seatunnel/pull/11307#issuecomment-6538830589
Thanks for the update — the latest review pass over the full diff at
5184b37259aa 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.
<!-- streview-comment:2164 -->
--
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]
