SEZ9 commented on PR #11636:
URL: https://github.com/apache/seatunnel/pull/11636#issuecomment-5225133356
Thanks @DanielLeens for going back to the primary evidence instead of taking
my review at face value — that's exactly the kind of re-verification I'd hope
for. I've re-checked both points against the links you shared, and I agree on
both.
**Issue 1 — withdrawn.** The linked jobs on `8c3765f5` (`unit-test (11,
windows-latest)` / job 92324201657 and the Java 8 leg / 92324201693) show step
4 correctly skipped via the `runner.os != 'Windows'` gate and step 5 completing
with real Surefire output over ~59 minutes. My review assumed the Windows path
was unreachable and that `./mvnw` would fail at shell invocation — the actual
run disproves both. Not a blocker, and not dead code.
**Issue 2 — withdrawn as a blocker.** You're right: the only `jacoco` hit in
`pom.xml` is the pre-existing `jacoco-agent.destfile` system property, and
there's no `jacoco-maven-plugin` / `prepare-agent` execution anywhere in the
repo, so nothing dynamically populates `argLine` today and there's nothing for
`<argLine>${surefire.jvm.args}</argLine>` to clobber. I'll downgrade this to a
non-blocking note: if coverage tooling is added later, this line will need
revisiting (e.g., the `@{argLine}`-prepend pattern with a defaulted property),
but that's future work, not this PR's problem.
@srijan-singh, that leaves two concrete asks before I re-approve:
1. **Timeout bump (210 → 240 for `all-connectors-it-3`)** — could you link a
recent run showing how close the job actually gets to the current limit? I want
to confirm we're buying legitimate headroom rather than masking a growing
slowdown. If there's a known cause for the increased runtime, a tracking issue
reference in the PR description would be enough.
2. **Green `Build` check on the current head** — once CI is fully green on
the latest commit, ping me and I'll do the re-review promptly.
Thanks both — the memory-split approach itself remains sound, and the
validation done here has been genuinely helpful.
<!-- streview-comment:94 -->
--
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]
