SEZ9 commented on PR #11307:
URL: https://github.com/apache/seatunnel/pull/11307#issuecomment-5658269732
Thanks for the update. Since `2b832dc0e82` is a two-parent merge
(`ee21221df89` + `35b2716cde7`) that syncs with `dev`, and the re-check
confirmed this PR's own hunks came through intact — including the
`seatunnel-spark-3.5-starter` module wiring and the
`seatunnel-shade-hadoop3-uber` exclude in
`seatunnel-core/seatunnel-spark-starter/pom.xml` — the merge itself isn't a
concern.
It also doesn't appear to address the findings from the review of
`ee21221df89c`. Could you confirm the status of each, or push follow-up commits?
**Blocking (HIGH)**
- F1 – `start-seatunnel-spark-3.5-connector-v2.cmd`: `setlocal
disabledelayedexpansion` runs before `!errorlevel!`/`!CMD!` are expanded, so
the branches compare literal strings and `call` targets the literal text
`!CMD!`. Please fix the ordering and share a quick manual run on Windows.
**Medium**
- F2 – `start-seatunnel-spark-3.5-connector-v2.sh`: the `eval` of the
assembled command line executes content built from user-supplied config values.
Please avoid `eval` here, or explain why it's needed.
- F6 – same script: `args=$@`, `${args}`, and `${CLASS_PATH}` are unquoted,
so argument quoting is lost and they're subject to word-splitting/globbing.
Please quote them.
- F5 – `.cmd` launcher: it exits 0 when the starter java process fails with
no stdout, and `for /f` doesn't surface java's real exit code. Please propagate
a non-zero exit.
- F4 – `seatunnel-core/seatunnel-spark-starter/pom.xml`: both
`log4j-slf4j-impl` and `log4j-slf4j2-impl` are in the logging jar include list
while both launchers put `starter/logging/*` on the classpath. Please keep only
the binding matching the SLF4J version on the 3.5 classpath, or explain how the
conflict is avoided.
- F3 – `seatunnel-spark-3.5-starter/pom.xml`: shading
`seatunnel-translation-spark-3.3` (compiled against Spark 3.3.0) means Catalyst
binary incompatibilities surface only at runtime. The reflection shim approach
is fine for this PR if documented as a known limitation — please confirm that's
the intent.
- F7 – tests / `docs/en/getting-started/locally/quick-start-spark.md`: the
docs point Spark 3.5 users at the streaming template, but the new 3.5 tests
only cover the row encoder and batch write. Please add a micro-batch streaming
test on 3.5.8, or adjust the docs.
- F8 – `docs/en/engines/spark.md`: narrowing `seatunnel-spark-3-starter.jar`
to "Spark 3.3.x" leaves Spark 3.4.x users without guidance. Please state which
starter they should use.
If any of these are already handled in a commit I'm missing, point me to it
and I'll re-check.
<!-- streview-comment:1030 -->
--
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]
