SEZ9 commented on PR #11307: URL: https://github.com/apache/seatunnel/pull/11307#issuecomment-5770427691
Thanks @goutamadwant for the update. Fixing the Windows argument capture, the classpath wildcard construction and the launcher-owned temporary-file assertions lines up with the Windows launcher and shell quoting points from the earlier review, so this is heading the right way. A few things I still need before I can close those out: 1. **Point me at the new head.** The last references I have in this thread are `b1484e581` and `457a0d116`; please share the commit that carries the argument-capture / classpath-wildcard / temp-file changes so I can re-diff exactly that increment. 2. **Windows `.cmd` (F1 / F5).** Please confirm the delayed-expansion ordering in the Spark 3.5 `.cmd` launcher is now correct so the job command is actually executed rather than compared as literal text, and whether the launcher now propagates a non-zero exit when the starter JVM fails with no stdout instead of exiting 0. If the native-Windows packaged-launcher tests cover both, a short note on which cases do is enough. 3. **Shell launcher (F2 / F6).** Please confirm the shell launcher no longer evaluates a command line assembled from job config values, and that the arguments and classpath are now quoted / array-based so a job path with spaces or glob characters survives intact. 4. **Logging jar list (F4).** Are both `log4j-slf4j-impl` and `log4j-slf4j2-impl` still in the starter logging include list? If so, please pick one for the Spark 3.5 starter or explain how the dual binding is avoided on the plain-java classpath. 5. **Translation layer compatibility (F3).** Since the 3.5 starter still shades `seatunnel-translation-spark-3.3`, please confirm the E2E run on 3.5.8 exercises the DataSource V2 write path end-to-end so any Catalyst binary mismatch would surface in CI rather than at user runtime. 6. **Streaming coverage (F7) and docs (F8).** Please either add a micro-batch streaming case to the Spark 3.5 tests or adjust `quick-start-spark.md` so it does not point 3.5 users at the streaming template as the first example, and add a line in `docs/en/engines/spark.md` telling Spark 3.4.x users which starter to use. Once I have the commit reference I will re-review just that delta against the points above. <!-- streview-comment:1223 --> -- 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]
