DanielLeens commented on PR #11851:
URL: https://github.com/apache/seatunnel/pull/11851#issuecomment-5342883403
@davidzollo thanks for taking a look, and I don't want to be the one holding
up a contributor's PR without cause — but I need to push back here rather than
let this pass quietly, since I noticed the merge already happened a few seconds
after your approval.
I re-verified my concern directly against the merge commit itself
(`f415ed8e52630444bfc3a40d0b8fc7c3a38051ab`, now on `dev`), not just the PR
diff, to make sure nothing had changed since my review yesterday:
- `pom.xml` on the merged `dev` HEAD still declares
`<module>seatunnel-shade</module>` (line 56) — it is still an in-repo,
in-lockstep-versioned (`${revision}`) Maven module, not an externally published
dependency.
- The actual migration proposal to pull `seatunnel-shade` out into its own
repo, #9993, is still **open and unmerged** as of right now.
- The merge diff is confirmed to be the exact inverse of #11673: it removes
the `com.fasterxml.jackson` →
`${seatunnel.shade.package}.hadoop.com.fasterxml.jackson` relocation from
`seatunnel-shade/seatunnel-hadoop-aws/pom.xml`, and deletes
`tools/dependencies/check_shaded_jackson_refs.py` outright (not just disables
it).
So the stated rationale for the revert ("seatunnel-shade is no longer a
submodule here, changes are ineffective") is still factually incorrect against
current `dev`, and this has now merged. Concretely, that means `dev` currently
reintroduces the `NoSuchMethodError` on
`org.apache.hadoop.util.JsonSerialization.getMapper()` that #11673 fixed
(reproduced there with `javap` evidence), for anyone using
`fs.s3a.assumed.role.*` with the officially distributed `seatunnel-hadoop-aws`
jar — and the CI check that would have caught this is gone, not just green.
Since it's already merged, I'm not asking to block anymore — I'm asking for
a prompt follow-up: either revert this revert until #9993 actually lands, or
re-apply the Jackson relocation (and ideally restore the regression script) in
a new PR. Happy to be shown I've misread something, but the module list and
#9993's open state are both directly checkable right now, so if I'm wrong about
the premise I'd like to understand where.
--
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]
