SEZ9 commented on PR #11307:
URL: https://github.com/apache/seatunnel/pull/11307#issuecomment-5643023611
@goutamadwant thanks for the ping. Before an approving review, I'd like to
close the loop on the findings from the previous round, since the status
summary for `169864a0be9` (CI run `33731860742`) doesn't say which of them were
addressed:
- **F1 (HIGH)** – `start-seatunnel-spark-3.5-connector-v2.cmd`: `setlocal
disabledelayedexpansion` is issued before `!errorlevel!`/`!CMD!` are expanded,
so the branches compare literal strings and the `call` target is the literal
text `!CMD!`. Has this been fixed on the current head? If so, could you point
me at the change and confirm the `.cmd` was run end-to-end on Windows?
- **F5** – same file: a java failure with empty stdout exits 0, and `for /f`
hides the real exit code. Please confirm the failure path now propagates a
non-zero exit.
- **F2 / F6** – `start-seatunnel-spark-3.5-connector-v2.sh`: `eval` of
config-derived content, plus unquoted `args=$@`, `${args}`, `${CLASS_PATH}`. If
the intent is to mirror the existing Spark 3 launcher for consistency, please
say so and we can track hardening as a follow-up; otherwise please quote the
variables and avoid `eval`.
- **F3** – `seatunnel-spark-3.5-starter/pom.xml` shades
`seatunnel-translation-spark-3.3` (built against Spark 3.3.0). What testing
backs binary compatibility of the Catalyst-facing code with 3.5.8? A short note
in the PR description would be enough.
- **F4** – `seatunnel-spark-starter/pom.xml`: both `log4j-slf4j-impl` and
`log4j-slf4j2-impl` end up under `starter/logging/*`. Please confirm whether
both land in the same lib dir at runtime, or exclude the one not needed per
starter.
- **F7** – docs point Spark 3.5 users at the streaming template, but the new
tests only cover the row encoder and batch write. A micro-batch streaming test
on 3.5.8 would be good; if you'd rather defer it, please note that in the PR.
- **F8** – `docs/en/engines/spark.md` now says
`seatunnel-spark-3-starter.jar` is for "Spark 3.3.x" and leaves 3.4.x users
without guidance. Please either widen the wording or add a line on which
starter 3.4.x should use.
A quick per-item "fixed in this head / deferred with reason" list would let
me do a focused final pass. Once F1 in particular is confirmed, I'm happy to
move this forward.
<!-- streview-comment:986 -->
--
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]
