SEZ9 commented on PR #11307:
URL: https://github.com/apache/seatunnel/pull/11307#issuecomment-5691929432
Thanks @goutamadwant — the logging-packaging update is useful. Reading the
probe result (single SLF4J 1.7 binding, Log4j working on Java 8 and 11 against
freshly built current-head artifacts) as addressing PR11307-F4, with two
follow-ups:
- **F4 (dual SLF4J binding)** — can you state exactly which of
`log4j-slf4j-impl` / `log4j-slf4j2-impl` ends up under `starter/logging/` in
the archive, and how the other is kept out (removed from the include list vs.
excluded elsewhere)? The probe shows the runtime outcome; I'd like the
packaging rule that guarantees it visible in
`seatunnel-core/seatunnel-spark-starter/pom.xml` so it doesn't regress.
- **F7 (streaming on 3.5)** — agreed with your framing that this is
packaging validation only, so F7 stays open. Please add a micro-batch streaming
run through the Spark 3.5 starter (a test alongside the existing
row-encoder/batch-write coverage, or at minimum a log of the streaming template
running on 3.5.8) before I mark it resolved.
On the rest of the earlier scope, current status as I have it:
- **F1 / F5 (Windows `.cmd` launcher)** — no update in the thread yet.
Please confirm the `setlocal disabledelayedexpansion` ordering and the
exit-code propagation out of the `for /f` loop are fixed on the current head,
ideally with a short Windows run showing a failing starter java process
producing a non-zero exit.
- **F2 / F6 (`eval` and unquoted `$@`/`${CLASS_PATH}` in the `.sh`
launcher)** — the re-review describes the command string now being passed as a
NUL-delimited argument file executed as a real argv array. Please confirm that
is what is on the current head and that the `args`/`CLASS_PATH` expansions are
quoted as well; a run with a config path containing spaces would close both.
- **F3 (shading `seatunnel-translation-spark-3.3` into the 3.5 starter)** —
the `SparkRowEncoder` reflection shim covers the known `RowEncoder.apply`
break; please add a short note (docs or pom comment) that the 3.5 starter
reuses the 3.3 translation layer so future Catalyst breaks are easy to trace.
- **F8 (Spark 3.4 guidance)** — the re-review says 3.4 users are now
explicitly told to stay on the 3.3 jar; please confirm that text is in
`docs/en/engines/spark.md` on the current head.
Re the retried Maven-bootstrap failure and the AssertSink / engine / CDC
failures you mention as still outstanding: once you have determined which of
those are unrelated to this PR, please list them explicitly so we can separate
them from anything the new module triggers.
<!-- streview-comment:1092 -->
--
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]
