DanielLeens commented on PR #9993:
URL: https://github.com/apache/seatunnel/pull/9993#issuecomment-4550420244
Thanks for pushing this forward. I re-reviewed the latest head from scratch
against the real consumer paths in the repository, not just the root POM diff.
## What this PR is trying to solve
- User pain point: `seatunnel-shade` is still built inside the main
SeaTunnel reactor today, which ties shade dependency evolution and release
cadence to the main repository.
- Fix approach: this PR moves the main repo from “build the shade jar here”
to “consume externally published `seatunnel-shade-*` artifacts”, and updates
POMs, docs, and E2E helpers around that new contract.
- One-line summary: the direction is right, but the current head still
leaves the shade-artifact contract half-migrated across starters, docs, and
E2E, so it is not ready to merge yet.
## Runtime / consumer path I re-traced
```text
root pom
-> remove seatunnel-shade from reactor
-> introduce new seatunnel.shade.* version properties
starter packaging
-> seatunnel-starter / flink-starter / spark-starter shade excludes
-> these must exclude the same artifact family the new contract now
publishes
E2E packaging
-> MavenJarUtil.getHadoop3UberJarPath() resolves the host-side jar
-> AbstractTestContainer / KubernetesIT decide the container-side target
path
user deployment path
-> docs/en + docs/zh tell users which jar to place into
${SEATUNNEL_HOME}/lib
```
## Findings
### Issue 1: starter excludes still point at 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`
- Severity: High
- Why this is a problem:
- The root POM has already moved to the new `seatunnel-shade-*` contract,
but the starter packaging boundary still excludes the old artifact
`org.apache.seatunnel:seatunnel-hadoop3-3.1.4-uber`.
- That means the production dependency contract and the starter packaging
contract are still not the same contract.
- Risk:
- Once the new shade line is actually consumed everywhere, starter
packaging can drift away from the real dependency naming and leave behind
hidden assembly problems.
- Best fix:
- Update the starter excludes in the same PR to the new artifact naming.
If there is still a compatibility window for the old name, document that window
explicitly instead of mixing old and new rules.
### Issue 2: multiple user-facing docs still tell people to use the old
`2.3.13` jar names
- Location:
- `docs/en/connectors/source/Hive.md:27`
- `docs/en/connectors/sink/Hive.md:15`
- `docs/en/connectors/source/OssFile.md:22`
- `docs/en/connectors/sink/OssFile.md:22`
- `docs/en/engines/zeta/hybrid-cluster-deployment.md:351`
- `docs/en/engines/zeta/separated-cluster-deployment.md:312`
- and the corresponding `docs/zh/*` files
- Severity: High
- Why this is a problem:
- The PR publicly moves the project toward the new external shade artifact
line, but the docs still instruct users to install the old
`seatunnel-shade-hadoop3-uber-3.1.4-2.3.13.jar` naming contract.
- Real users follow the docs, not the root POM.
- Risk:
- Users can deploy with the old jar naming while the code and tests are
already trying to move to the new one.
- Best fix:
- Update `docs/en` and `docs/zh` together so they describe the same new
artifact contract end to end. If the old jar name is still intentionally
supported, please document the compatibility story explicitly.
### Issue 3: the E2E container-side shade jar name is still hard-coded to a
literal version
- Location:
-
`seatunnel-e2e/seatunnel-e2e-common/src/test/java/org/apache/seatunnel/e2e/common/container/AbstractTestContainer.java:56-57`
-
`seatunnel-e2e/seatunnel-engine-e2e/seatunnel-engine-k8s-e2e/src/test/java/org/apache/seatunnel/engine/e2e/k8s/KubernetesIT.java:189-190`
- Severity: Medium
- Why this is a problem:
- The host-side lookup now goes through
`MavenJarUtil.getHadoop3UberJarPath()`, but the container-side destination is
still the literal string `seatunnel-shade-hadoop3-uber-3.1.4-3.0.0.jar`.
- So the two ends of the E2E contract are still not derived from one
shared version source.
- Risk:
- The next shade version change can break E2E again even if the host-side
lookup is updated correctly.
- Best fix:
- Derive the container-side destination name from the same shared
version/property source instead of hand-writing a literal version.
- Test stability note:
- Because this PR changes E2E helper code, I also checked the stability
angle. My rating here is **Medium risk** rather than High risk: the E2E flow is
not flaky because of timing, but it is still brittle because the container-side
artifact contract is hard-coded.
### Issue 4: the current Build is red
- Location:
- GitHub Actions `Build`
- fork run `26449224731`
- Severity: Medium
- Why this is a problem:
- On the current head, I could confirm failing jobs in the referenced fork
workflow, including:
- `all-connectors-it-5 (8, ubuntu-latest)`
- `rocketmq-connector-it (8, ubuntu-latest)`
- `paimon-connector-it (8, ubuntu-latest)`
- Some parallel legs were also cancelled, so the current CI signal is not
settled yet.
- Risk:
- The head is still not in a green mergeable state.
- Best fix:
- First close the contract blockers above, then rerun Build on the
cleaned-up head. If the failures persist, inspect whether any of them are now
hitting the shade-artifact split between host/container/docs/starter consumers.
## Code quality / tests / docs
- Code style itself looks fine. The real issue is contract closure, not
formatting.
- Test stability rating for the changed E2E code: **Medium risk**, because
the container-side artifact naming is still hand-written.
- Docs absolutely need to be part of the fix here because this PR changes a
user-visible packaging/deployment contract.
## Conclusion
### Conclusion: can merge after fixes
1. Blockers
- Issue 1: align the starter packaging excludes with the new shade artifact
naming.
- Issue 2: align `docs/en` and `docs/zh` with the new shade artifact
contract, or document the compatibility window explicitly.
2. Suggested follow-ups
- Issue 3: remove the hard-coded container-side shade jar version from the
E2E helpers.
- Issue 4: rerun and recheck Build after the contract is closed.
Overall, I agree with the long-term direction of the PR, but the blocker
family from earlier Daniel reviews is still present on the latest head: the
repository has started the migration, but the real consumers still do not all
follow one consistent contract 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]
