test: verify Celeborn reflection compatibility - #5604
Conversation
ziting-openai
left a comment
There was a problem hiding this comment.
Summary
Reviewed head cbaa94dd31ae0fa9bf39241418133597d219e6b4 against main at 6dedcfeee8777b578570e6304e514cc359df1de3, using five independent passes covering reflected contracts, version compatibility, test execution, initialization and ownership boundaries, and regression coverage. 0 P1 / 0 significant P2 findings; approve. Production shuffle behavior is unchanged.
Prior state and problem
The partition pusher and callback tracker depend on private Celeborn fields and public method signatures. Tests using stand-ins can validate Comet's behavior while missing a mismatch with the released client jars.
Design approach
An opt-in Maven profile adds the shaded Celeborn client as a test dependency and registers a separate test source directory. Four reflection-only tests check the released artifact, partition-pusher signatures, push-state counters, and transport callback interfaces. A dedicated Linux matrix runs them against Celeborn 0.6.0 and 0.7.0 with Spark 3.5 / Scala 2.12.
Correctness / compatibility analysis
The checked signatures match the partition pusher and callback tracker, including the four- versus five-argument mapperEnd, the 0.7 CRC method and crypto field, exact atomic counter types, and shaded transport interfaces. Bytecode inspection of both released jars also confirms the declared retry-executor and bootstrap-list types accept the wrappers used by Comet.
I compiled the exact new suite without modification and ran it in separate JVMs against each released jar: 4/4 tests passed for each version. The standalone harness used Scala 2.12.18, JDK 21, ScalaTest 3.2.19, and the Spark 3.5 logging API. The repository suite-inventory check and structural POM/workflow checks passed.
The exact-head CI jobs also passed all four tests for Celeborn 0.6.0 and 0.7.0 on JDK 17. Other native and Spark jobs were still running when checked. The separate PyArrow Spark 4.1 failure is an HTTP 429 while downloading Maven 3.9.6, before its build or tests begin. I did not rerun the full Comet native/JVM matrix locally.
Key design decisions
The profile has no automatic activation, the dependency has test scope, and the source directory is registered only by that profile. Ordinary builds therefore retain their existing dependency and test-source paths. Loading classes without initialization keeps this check independent of a live Celeborn service; it checks reflection compatibility, not end-to-end shuffle semantics.
Implementation sketch
- Activate the compatibility profile with a selected Celeborn version.
- Compile the isolated suite against the released shaded client.
- Resolve and assert the required fields, methods, and version-specific contracts.
- Run both versions in the dedicated CI matrix while keeping the suite out of the generic matrix.
Behavioral changes worth calling out
Only opt-in test execution and CI coverage change. No production dependency, shuffle implementation, or native code is added or modified.
Suggested improvements
No actionable P1 or significant P2 findings survived validation. No inline comments are needed.
sunchao
left a comment
There was a problem hiding this comment.
Reviewed cbaa94dd31ae0fa9bf39241418133597d219e6b4 against base/merge-base 6dedcfeee8777b578570e6304e514cc359df1de3. No introduced or materially worsened P1/P2 found.
Fresh validation on Spark 3.5.9 / Scala 2.12.18 / JDK 17: four compatibility tests passed against each released Celeborn 0.6.0 and 0.7.0 client, plus 73 profile-free pusher/manager tests. The suite also passed without project fixtures and failed as expected when the client dependency was removed. Exact-base/head Maven model checks and packaged binary/source JAR inspection confirmed dependency and test-source isolation.
Both dedicated compatibility CI jobs passed. The separate PyArrow Spark 4.1 job failed with HTTP 429 while downloading Maven 3.9.6. Other CI jobs are still running. This verifies reflection contracts and build isolation, not native shuffle transport or deployed-cluster behavior.
|
Merged, thanks @pingzh ! |
Which issue does this PR close?
Closes #5530.
Rationale for this change
The native Celeborn shuffle path reflects on private Celeborn members. Its ordinary unit tests use hand-written stand-ins, so they cannot detect when a released Celeborn client changes one of those fields or method signatures.
What changes are included in this PR?
celeborn-reflection-compatibilityMaven profile with a test-scoped shaded Celeborn client dependency and an isolated test source directory.mapperEnd, CRC, and crypto differences.How are these changes tested?
python3 dev/ci/check-suites.py.git diff --check.