SEZ9 commented on PR #11174: URL: https://github.com/apache/seatunnel/pull/11174#issuecomment-6259008918
Thanks for the update — I re-checked head `31000f5a88dc8d2b505a86a3ad4fae214c431c54` against the open points from the previous review, and they still stand. Summary of what remains: **Docs / artifact model (F1, F5, F6)** - `speed-limit.md` and `JobEnvConfig.md` now declare features unsupported on *all* Spark versions. That is a cross-engine contract change and does not belong in a Spark 4.1 PR — please scope those statements to Spark 4.1 only, or split them into a separate PR. - `engines/spark.md` says `seatunnel-transforms-v2` is **not** shipped in the `-spark41-bin` tarball, while the PR description says the tarball contains transforms. Please pick one model and make the assembly, the docs and the description agree. - `quick-start-spark.md` tells users to "Create" `config/spark41-fake-to-console.conf`, but the repo now ships that file. Please change the wording to reference the shipped file, and confirm the spark41 assembly actually packages `config/` so the documented command works. Regarding @nzw921rx's point on the JDK 11/17 upgrade: if that removes the need for a separate Spark 4.1 distribution, it would also resolve F5/F6 at the root. Could you state in the PR description which direction you intend (separate `-spark41-bin` vs. the standard distribution), and whether JDK 11 can actually run the Spark 4.1 starter? That decision drives the remaining docs/packaging fixes. **Robustness (F2, F3)** - `spark-4-1-it` builds the starter with `package`, not `install`, so the second reactor cannot resolve `seatunnel-spark-4.1-starter` from the local repo. Please switch to `install` (or run both in one reactor). - Transform jobs on the `-spark41-bin` tarball fail only at runtime with an obscure error. The starter should validate the job config and fail fast with a clear message rather than relying on the docs warning in `engines/spark.md`. **Test wiring (F4, F7)** - `SparkCommandArgsTest` and `MultiTableSinkTest` are added to `seatunnel-spark-4.1-starter`, but no CI lane runs them. Please wire the starter's unit tests into a lane, or drop them. - The `spark-4-1-it` smoke lane can silently no-op: `-Dit.test=FakeSpark41IT -DfailIfNoTests=false` combined with the `RUN_SPARK_41_CONTAINER` gate means a skipped `FakeSpark41IT` still reports success. Please remove `-DfailIfNoTests=false` for that lane (or otherwise assert the test actually executed) so the lane proves the IT ran. **Workflow actions (F8)** - The new CI jobs use `actions/checkout@v2` and `actions/setup-java@v3`. Please align them with the versions used elsewhere in `backend.yml`. Once those are addressed, please push a new head and share the CI run for it so I can verify the `spark-4-1-it` lane end to end. <!-- streview-comment:1877 --> -- 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]
