SEZ9 commented on PR #11307: URL: https://github.com/apache/seatunnel/pull/11307#issuecomment-6392431929
Thanks @goutamadwant for the update and the summary. Since I can only confirm fixes from the diff itself, here is what I'd like to pin down for each earlier point: - **F1 / F5 (Windows launcher)** – "Windows exit-code handling" sounds like F5. Could you confirm F1 is also fixed, i.e. `setlocal disabledelayedexpansion` no longer runs before `!errorlevel!`/`!CMD!` are expanded, so the generated command is actually executed? Since the packaged tests have 4 Windows-only skips and native Windows execution is still pending, a pasted run from a real Windows box (success path plus a forced non-zero exit from the starter java process) would settle both. - **F2 / F6 (Unix launcher)** – "safer Unix argument handling" sounds like F6 (quoting). Is the command still run through `eval` (F2), or executed directly now? If `eval` is still there I'd like it removed before merge. - **F3 (shaded 3.3 translation layer)** – does the Spark 3.5 starter still shade the Spark 3.3-compiled translation layer? If so, what does the runtime row-encoder compatibility change cover, and which Catalyst incompatibilities remain that the documented compatibility limits call out? - **F4 (logging jars)** – what do the "packaged logging checks" assert? I'm specifically looking for a check that only one of `log4j-slf4j-impl` / `log4j-slf4j2-impl` ends up on the `starter/logging/*` classpath for each starter. - **F7 (streaming coverage)** – appreciate the honesty that the micro-batch test covers the sink adapter rather than streaming through the public starter. That's acceptable for this PR provided the docs say the same — could you point me to where the Spark 3.5 docs tell users the streaming template is not validated end-to-end on 3.5.8 yet? - **F8 (Spark 3.4 guidance)** – could you point me to the EN/ZH doc text that now covers Spark 3.4.x users? Before merge I'd also want green CI on the merged head, since local checks weren't rerun after the latest dev merge, plus the Windows run above. Once those are in I'm happy to do the final re-review. Thanks for the thorough work on this. <!-- streview-comment:2012 --> -- 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]
