DanielLeens commented on PR #11636: URL: https://github.com/apache/seatunnel/pull/11636#issuecomment-5203569385
Thanks for the thorough second pass, @SEZ9 — I appreciate you re-verifying independently rather than taking my APPROVED at face value. I went back through your two "must fix" findings (Issues 1 and 2) against the actual CI evidence and the current `pom.xml` on this same head (`8c3765f5`), and I don't think either one holds up. Sharing what I found so we're working from the same facts. **Issue 1 (Windows step invokes `./mvnw`, will fail / is dead code).** This already ran, successfully, on `8c3765f5` — not hypothetically. Job `Run / unit-test (11, windows-latest)` (https://github.com/srijan-singh/seatunnel/actions/runs/31011149808/job/92324201657) shows step 4 "run all modules unit test" **skipped** (correctly gated by `if: runner.os != 'Windows'`) and step 5 "run all modules unit test (Windows)" completed with **conclusion: success**, running ~59 minutes with real Surefire output in the log, e.g.: ``` [WARNING] Tests run: 3, Failures: 0, Errors: 0, Skipped: 2, ... - in org.apache.seatunnel.api.ChineseCharacterCheckTest ``` The `java 8` / `windows-latest` leg (job 92324201693) shows the identical pattern: step 4 skipped, step 5 success. So `./mvnw` does execute correctly under the Windows runner's default shell here — this isn't dead code and it isn't broken; it's demonstrated working on both Windows matrix legs on the exact commit under review. **Issue 2 (hard-coded `<argLine>` clobbers a JaCoCo-injected argLine, silently killing coverage).** I checked for JaCoCo wiring on this head and don't find any. `grep -n -i jacoco pom.xml` returns exactly one hit — the `<jacoco-agent.destfile>` entry under `systemPropertyVariables` in the surefire config — which is pre-existing context untouched by this diff, and there is no `jacoco-maven-plugin` (no `prepare-agent` execution) declared anywhere in the repository. Since nothing currently populates the `argLine` Maven property dynamically, there's no live agent injection for the new `<argLine>${surefire.jvm.args}</argLine>` to clobber, and no coverage collection running today that this PR could silently break. This matches what I found digging into the same property in my previous round. Happy to be pointed at a specific module/profile if I'm missing one, but I couldn't find it in the current tree. Issue 3 (`MAVEN_OPTS` at 1536m) is a fair watch-item, and one I raised myself in section 1.3 of my approval — already flagged as non-blocking given the actual green run at that heap size, and I'd still support bumping toward ~2048-3072m as a follow-up rather than a blocker. Issues 4-6 (property naming/scope, timeout-bump-as-stopgap, duplicated env block) are reasonable Low-severity nits I'd be glad to see picked up, but none of them change correctness or the actual CI outcome on this head. Given that, I'm not reinstating CHANGES_REQUESTED — my APPROVED stands. If Issue 1 or 2 turn out to reproduce somewhere I haven't looked, please point me at the exact job/log or module and I'll take another pass right away. -- 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]
