DanielLeens commented on PR #9993:
URL: https://github.com/apache/seatunnel/pull/9993#issuecomment-4538713976
Thanks for pushing this forward. I re-reviewed the latest head from scratch,
including the newest delta in AbstractTestContainer / KubernetesIT and the full
current consumer path around dist, E2E, K8s, and docs.
# What this PR fixes
- User pain: keeping the shade modules inside the main reactor makes the
build and release path heavier than necessary.
- Fix approach: this PR moves consumers toward externally published
seatunnel-shade-* artifacts.
- One-line summary: the direction is reasonable, but the current head is
still only a partial migration because the real consumer-side contract is not
closed yet.
# Runtime chain I checked
~~~text
Build side
-> pom.xml:528 switches consumers to seatunnel-shade-hadoop3-uber
-> seatunnel-dist/pom.xml:852,1068 and assembly-bin*.xml copy the new
artifact into dist/lib
Consumer side
-> docs/en/engines/zeta/hybrid-cluster-deployment.md:351 still documents
seatunnel-shade-hadoop3-uber-3.1.4-2.3.13.jar
-> seatunnel-engine-k8s-e2e/src/test/resources/seatunnel_dockerfile:20
still copies the 2.3.13 file name
-> MasterWorkerClusterSeaTunnelWithTelemetryIT.java:71 still expects the
2.3.13 file name
~~~
# Findings
Issue 1: the shade artifact contract is still mixed between old and new
consumer paths
- Location: docs/en/engines/zeta/hybrid-cluster-deployment.md:351;
seatunnel-engine-k8s-e2e/src/test/resources/seatunnel_dockerfile:20;
seatunnel-engine-e2e/.../MasterWorkerClusterSeaTunnelWithTelemetryIT.java:71
- Why this matters: the build side has already moved to the new artifact,
but real consumers still require the old 2.3.13 file name.
- Risk: user deployment, E2E startup, and K8s image assembly can still look
for the wrong jar even after the producer side was migrated.
- Better fix: centralize the final artifact contract and migrate dist, E2E,
K8s, and docs together.
- Severity: High
Issue 2: starter shade excludes still point to the old artifactId
- Location: seatunnel-core/seatunnel-starter/pom.xml:137;
seatunnel-core/seatunnel-flink-starter/pom.xml:96;
seatunnel-core/seatunnel-spark-starter/pom.xml:102
- Why this matters: the dependency side moved to
seatunnel-shade-hadoop3-uber, but the starter exclusions still reference
seatunnel-hadoop3-3.1.4-uber.
- Risk: the new shaded jar can accidentally leak into starter fat jars later.
- Better fix: update the excludes to the new artifactId or add a packaging
check proving the new jar only lands in dist/lib.
- Severity: High
# CI note
- The current Build check is still red on the latest head, so I would not
treat this branch as merge-ready yet.
# Merge conclusion
### Conclusion: can merge after fixes
1. Blocking items
- Issue 1: close the consumer-side artifact contract end to end.
- Issue 2: update the starter excludes to the new artifactId.
- Please also get the final Build green on the conflict-free head.
2. Suggested non-blocking follow-up
- If possible, keep the final jar name / destination logic behind a single
source of truth so the next shade-version bump does not reopen the same class
of drift.
Overall, I still agree with the migration direction, but the current head is
not a closed-loop migration yet.
--
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]
