DanielLeens commented on PR #12662:
URL: https://github.com/apache/seatunnel/pull/12662#issuecomment-6098922543
Thanks @Rangsh, and thanks @SwatiKumari250 for getting this started. I
re-checked the plan against the current `dev` and the #12655 thread, and the
direction looks right to me.
**Status of my four findings from the earlier review (head `5e17b58c698f`,
unchanged since):**
1. No root `surefire.module.args` on `dev`: resolved on the `dev` side.
#11545 merged on 2026-10-09, and the root `pom.xml` now defines
`surefire.module.args` (line 186) and uses `${surefire.jvm.args}
${surefire.module.args}` for both Surefire (line 798) and Failsafe (line 830).
It is fully cleared once this branch is rebased (it is currently 43 commits
behind `dev`, with no conflicts).
2. Duplicate ownership: resolved by the agreement on #12655, one
implementation, this PR.
3. ASF header blank lines: still open, your plan covers it.
4. JDK 17 verification evidence: still open, your plan covers it.
**One detail so the rebase push lands the final shape in one go.** On the
current head, `connector-lance/pom.xml` still has the `darwin-aarch64-java9+`
profile, which sets `surefire.jvm.args` to `...
--add-opens=java.base/java.nio=ALL-UNNAMED -Dos.arch=aarch64 -Dos.name=Mac OS
X`. That is a second copy of the module flags, so the "already done" part is
the generic `java9+` profile only. The plain `darwin-aarch64` profile already
activates on every JDK for Mac ARM, so I would remove `darwin-aarch64-java9+`
entirely instead of trimming it; the `--add-opens` then arrives through the
root `${surefire.module.args}`, and `darwin-aarch64` keeps only the `os.arch` /
`os.name` overrides.
**Process notes:**
- Maintainer edits are enabled on this branch, so pushing there is fine.
Since @SwatiKumari250 is the author, it would be nice to get a quick thumbs-up
from her on this thread, and to keep her original commit in the history.
- A push to the branch does not change the PR title or description. Those
can be edited by the PR author (or a committer), so @SwatiKumari250 could fill
in the description, or the verification output can simply be pasted as a
comment here. A title in the usual form, for example `[Improve][Connector-V2]
...`, would also help.
- About CI: as far as I can see, the `Build` check is `ACTION_REQUIRED` with
"Unable to detect the workflow run", and the fork `SwatiKumari250/seatunnel`
has no workflow runs at all. So this is not a maintainer-approval gate: GitHub
Actions has to be enabled in her fork (Actions tab, then enable workflows), and
only the fork owner can do that. Once it is enabled, a push to this branch will
trigger the run there. Until then, your local JDK 17 run with
`-Dsurefire.jvm.args` excluding the `--add-opens` values is the evidence we
need.
No new commit yet, so I will do the full re-review of the new head once the
rebased branch is pushed. Thanks again to both of you for coordinating so
nicely.
--
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]
