hawk9821 commented on PR #9993:
URL: https://github.com/apache/seatunnel/pull/9993#issuecomment-5186588138
> Thanks for working on this. I re-reviewed the latest head `c70b99f1d484`
from scratch.
>
> # What this PR fixes
> * User pain: The in-repo `seatunnel-shade` module (Guava, Jackson,
Hazelcast, Hadoop uber, etc.) rarely changes yet had to be rebuilt and released
with every SeaTunnel release, inflating build times and coupling the release
cycles.
> * Fix approach: Delete the entire `seatunnel-shade` module tree (including
the patched Hazelcast sources under `com.hazelcast.**`), pin
`seatunnel.shade.version=3.0.0` plus per-library version properties in the root
`pom.xml` `<dependencyManagement>`, consume `seatunnel-shade-*` artifacts from
Maven Central, and update docs (new `shade-guide.md`, jar names like
`seatunnel-hadoop3-3.1.4-uber.jar` →
`seatunnel-shade-hadoop3-uber-3.1.4-3.0.0.jar`), CI, and LICENSE accordingly.
> * One-line summary: The refactor is structurally sound and the docs are
thorough, but I need explicit confirmation that the deleted Hazelcast override
classes (`MemberImpl`, `ClusterServiceImpl`, `MembershipManager`) — which carry
SeaTunnel-specific cluster-membership patches — are byte-for-byte carried into
the published `seatunnel-shade-hazelcast:5.1-3.0.0` artifact, since
`LiteNodeDropOutTcpIpJoiner` in the engine still depends on that patched
behavior; the `ScalaCompilerVersionCheckTest` change and the ~24h Central
propagation window called out in `shade-guide.md` also need attention before
merge.
>
> # Runtime chain I rechecked
> ```
> SeaTunnelServer startup -> Hazelcast node bootstrap (now resolved from
org.apache.seatunnel:seatunnel-shade-hazelcast:5.1-3.0.0 instead of in-repo
seatunnel-hazelcast-shade)
> SeaTunnelServerStarter.createHazelcastInstance() ->
HazelcastInstanceFactory.newHazelcastInstance() SeaTunnelServerStarter.java
> -> Node.<init> instantiates ClusterServiceImpl (previously overridden
in
seatunnel-shade/seatunnel-hazelcast/seatunnel-hazelcast-shade/src/main/java/com/hazelcast/internal/cluster/impl/ClusterServiceImpl.java
— DELETED in this diff)
> -> ClusterServiceImpl creates MembershipManager (previously
overridden in .../internal/cluster/impl/MembershipManager.java — DELETED in
this diff)
> -> MembershipManager tracks MemberImpl instances (previously
overridden in .../cluster/impl/MemberImpl.java — DELETED in this diff)
> -> LiteNodeDropOutTcpIpJoiner.join() reads cluster membership from
ClusterServiceImpl/MembershipManager
seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/LiteNodeDropOutTcpIpJoiner.java
(still listed in LICENSE, unchanged in this PR — depends on the patched shaded
classes now sourced from Maven Central)
> Verification path: root pom.xml <dependencyManagement> pins
seatunnel-shade-hazelcast to
${seatunnel.shade.hazelcast.version}-${seatunnel.shade.version} (5.1-3.0.0);
seatunnel-ci-tools/src/test/java/org/apache/seatunnel/api/ScalaCompilerVersionCheckTest.java
updated to validate the scala-compiler shade artifact coordinates rather than
the removed local module.
> ```
>
> # Findings
> **Issue 1: Possible public API change in
seatunnel-ci-tools/src/test/java/org/apache/seatunnel/api/ScalaCompilerVersionCheckTest.java**
>
> * Location:
`seatunnel-ci-tools/src/test/java/org/apache/seatunnel/api/ScalaCompilerVersionCheckTest.java`
> * Why it matters: A public/protected signature appears to be removed or
changed. Verify backward compatibility for downstream connectors.
> * Severity: High
>
> **Issue 2: Possible public API change in
seatunnel-shade/seatunnel-hazelcast/seatunnel-hazelcast-shade/src/main/java/com/hazelcast/cluster/impl/MemberImpl.java**
>
> * Location:
`seatunnel-shade/seatunnel-hazelcast/seatunnel-hazelcast-shade/src/main/java/com/hazelcast/cluster/impl/MemberImpl.java`
> * Why it matters: A public/protected signature appears to be removed or
changed. Verify backward compatibility for downstream connectors.
> * Evidence:
`seatunnel-shade/seatunnel-hazelcast/seatunnel-hazelcast-shade/src/main/java/com/hazelcast/internal/cluster/impl/ClusterServiceImpl.java`;
`seatunnel-shade/seatunnel-hazelcast/seatunnel-hazelcast-shade/src/main/java/com/hazelcast/internal/cluster/impl/MembershipManager.java`
> * Severity: High
>
> # Review conclusion
> ### Conclusion: can merge after the blocking items are fixed
> **1. Blocking items**
>
> * Issue 1: Possible public API change in
seatunnel-ci-tools/src/test/java/org/apache/seatunnel/api/ScalaCompilerVersionCheckTest.java
> * Issue 2: Possible public API change in
seatunnel-shade/seatunnel-hazelcast/seatunnel-hazelcast-shade/src/main/java/com/hazelcast/cluster/impl/MemberImpl.java
>
> Nice progress overall — once the blocking points above are addressed this
should be in good shape. Happy to discuss.
Issue 1:
[seatunnel-ci-tools/.../ScalaCompilerVersionCheckTest.java](https://github.com/apache/seatunnel/pull/seatunnel-ci-tools/.../ScalaCompilerVersionCheckTest.java)
— not a public API change
The file is new (new file mode 100644), not modified. seatunnel-ci-tools is
a CI-only module (its
[pom.xml](https://github.com/apache/seatunnel/pull/pom.xml) depends only on
javaparser-core / javaparser-symbol-solver-core in test scope, and no other
module in the reactor references it). The class lives under src/test/java and
is a build-time regression guard, not a public API contract. I'd suggest
removing this from the blocker list.
Issue 2: Hazelcast shade deletions — not a breaking API change
I queried both repos via the GitHub API:
Main repo dev branch,
[seatunnel-shade/seatunnel-hazelcast/seatunnel-hazelcast-shade/...](https://github.com/apache/seatunnel/pull/seatunnel-shade/seatunnel-hazelcast/seatunnel-hazelcast-shade/...)
→ 4 .java files (MemberImpl, ClusterServiceImpl, MembershipManager, MemberMap).
Standalone repo main,
[seatunnel-shade-hazelcast/...](https://github.com/apache/seatunnel/pull/seatunnel-shade-hazelcast/...)
→ the same 4 .java files, with identical package paths and class names.
--
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]
