DanielLeens commented on PR #9993:
URL: https://github.com/apache/seatunnel/pull/9993#issuecomment-4518062065
Thanks for pushing the shade-module refactor forward. I pulled the latest
head `1591fdf939525c4233fceae4e28cf515c87ee551` locally as
`seatunnel-review-9993`, reviewed the full diff against `apache/dev`, and
checked the real dist / starter / E2E consumer paths instead of only the POM
changes.
# What this PR fixes
- User pain: keeping the shaded dependencies inside the main reactor
increases maintenance and release cost.
- Fix approach: move toward consuming externally published shaded artifacts
and update the surrounding build/dist/E2E paths.
- In one sentence: this PR is trying to migrate SeaTunnel from in-repo
shaded artifact production to external shaded artifact consumption.
# Runtime path I checked
```text
Artifact production side
-> root/module POMs move toward external shaded artifact consumption
-> MavenJarUtil.getHadoop3UberJarPath()
-> now resolves the jar from Maven coordinates/properties
Real consumer side
-> SeaTunnelContainer / ConnectorPackageServiceContainer /
ClusterSeaTunnelEngineContainer
-> still copy or expect the old shaded jar name in lib/
-> seatunnel-engine-k8s-e2e Dockerfile / KubernetesIT
-> still copy the old jar name
-> starter POM excludes / user docs
-> still reference the old artifact id / old jar naming contract
```
# Code review
## 1. Core logic
The migration direction is reasonable, and
`MavenJarUtil.getHadoop3UberJarPath()` is a useful step because it moves the
source jar lookup to Maven coordinates.
But the migration is still incomplete on the real consumer side. The latest
head still hardcodes the old shaded Hadoop uber jar name in multiple
E2E/container/K8S paths, and the starter excludes and user-facing deployment
docs still point to the old artifact contract as well.
So the production side is being moved, but the consumers have not been
migrated as a single closed loop yet.
## 2. Compatibility
This is a compatibility-sensitive migration even though there is no public
API change. The shaded jar file name and artifact id are part of the practical
contract for dist assembly, starter shading, container startup, K8S images, and
user deployment docs. Right now those consumers are not aligned with the new
production side.
## 3. Performance / side effects
No meaningful runtime CPU/memory concern here. The real side effect is
operational breakage: E2E, container startup, downstream shading, and user
deployment instructions can all continue looking for the wrong jar.
## 4. Error handling / logging
Issue 1: the E2E / container / K8S runtime paths still hardcode the old
shaded jar name, so the migration is not closed on the real consumer path
- Location: `seatunnel-e2e/.../SeaTunnelContainer.java:133-136`;
`.../ConnectorPackageServiceContainer.java:87,112,137`;
`.../ClusterSeaTunnelEngineContainer.java:80`; `.../KubernetesIT.java:190`;
`seatunnel-engine-k8s-e2e/src/test/resources/seatunnel_dockerfile:20`
- Why this is a problem: even though the source jar path is now resolved
from Maven coordinates, these runtime consumers still copy or expect the old
file name inside `${SEATUNNEL_HOME}/lib`.
- Risk: real integration paths can still fail or keep relying on the legacy
contract even after the production side moved.
- Best fix: centralize the final artifact file name and make the
container/K8S/E2E consumers all use that same single definition.
- Severity: High
- Already raised by others: No
Issue 2: starter excludes and user-facing docs still reference the old
artifact id / old jar naming contract
- Location: `seatunnel-core/seatunnel-starter/pom.xml:137`;
`seatunnel-core/seatunnel-flink-starter/pom.xml:96`;
`seatunnel-core/seatunnel-spark-starter/pom.xml:102`; multiple deployment/docs
paths such as `docs/en/connectors/source/Hive.md:27`,
`docs/en/engines/zeta/hybrid-cluster-deployment.md:351,385`, and the
corresponding zh docs
- Why this is a problem: the migration is not just about where the jar comes
from; it also changes what downstream consumers need to exclude, copy, and
document. Those consumers are still speaking the old contract.
- Risk: build-time and user-facing deployment behavior stay inconsistent
even if the core dependency declarations are updated.
- Best fix: finish the starter/doc migration in the same artifact contract,
or do not claim the shade migration is complete yet.
- Severity: High
- Already raised by others: No
# Testing coverage and stability
The risk here is not flaky tests. The risk is that the current validation
surface still clearly allows stale consumer paths to survive, which means the
migration is not yet fully enforced by the test/deployment matrix.
# Merge conclusion
### Conclusion: can merge after fixes
1. Blocking items
- Issue 1: migrate the real E2E / container / K8S consumers to the new
shaded artifact contract.
- Issue 2: migrate the starter excludes and the user-facing deployment/docs
contract to the same new artifact naming.
2. Suggested follow-up items
- None beyond the blockers above; the migration-closure gaps are the main
merge gate.
Overall, I agree with the direction, but the current head still looks like a
partial migration rather than a completed one. Happy to re-review once the
consumer side is migrated end to end.
--
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]
