SEZ9 commented on PR #11174: URL: https://github.com/apache/seatunnel/pull/11174#issuecomment-6244781824
Thanks for the update on `31000f5a8`. **`FakeSpark41IT` gating:** the environment-variable gate turns the earlier failure in the general `all-connectors-it-*` lanes into a clean skip, which is good. It does not close the other half of that finding though: in the dedicated `spark-4-1-it` lane, `-Dit.test=FakeSpark41IT -DfailIfNoTests=false` combined with the env gate still means a skipped or missing IT reports green. Please drop `-DfailIfNoTests=false` in that lane, or add an explicit check that `FakeSpark41IT` actually ran, so the smoke test cannot silently no-op. **On nzw921rx's comment about JDK 11/17:** if the JDK upgrade means Spark 4.1 no longer needs a separate `-spark41-bin` tarball, the docs/assembly findings (the `engines/spark.md` statement that `seatunnel-transforms-v2` is not shipped vs. the PR description saying "transforms" is included, and the quick-start wording around `config/spark41-fake-to-console.conf`) should be rewritten against the single distribution rather than patched. Could you confirm which direction you are taking — keep `-spark41-bin`, or fold Spark 4.1 into the regular distribution? Either way, docs, assembly and PR description need to describe the same artifact. That comment also ties into the fail-fast finding: whatever the packaging outcome, `start-seatunnel-spark-4.1-connector-v2.sh` / `SparkStarter` should reject an unsupported runtime (including transform jobs, if they remain unsupported) with a clear message at startup rather than an obscure runtime error. A docs warning alone isn't enough. Still outstanding from the previous review, with no change I can see on this head: - `speed-limit.md` / `JobEnvConfig.md` now mark features as unsupported on all Spark versions. Please scope those statements to Spark 4.1 only, or move the cross-engine change to a separate PR. - `spark-4-1-it` builds the starter with `package`; the second reactor needs `install` to resolve `seatunnel-spark-4.1-starter` from the local repo. - `SparkCommandArgsTest` and `MultiTableSinkTest` in `seatunnel-spark-4.1-starter` are not executed by any lane. Please wire them into `spark-4-1-it` or remove them. - The newly added CI jobs use deprecated / mutable-tag checkout and setup-java action versions; please bump them to the current pinned versions used elsewhere in the workflow. Once those are addressed I'm happy to take another full pass. <!-- streview-comment:1834 --> -- 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]
