Skip to content

test: verify Celeborn reflection compatibility - #5604

Merged
sunchao merged 1 commit into
apache:mainfrom
pingzh:pingzh-celeborn-reflection-compatibility-tests
Sep 1, 2026
Merged

test: verify Celeborn reflection compatibility#5604
sunchao merged 1 commit into
apache:mainfrom
pingzh:pingzh-celeborn-reflection-compatibility-tests

Conversation

@pingzh

@pingzh pingzh commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

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?

  • Add an opt-in celeborn-reflection-compatibility Maven profile with a test-scoped shaded Celeborn client dependency and an isolated test source directory.
  • Add a reflection-only suite that verifies every Celeborn field and method used by the partition pusher and callback tracker, including the version-specific mapperEnd, CRC, and crypto differences.
  • Run the suite in a dedicated Linux CI matrix against Celeborn 0.6.0 and 0.7.0.
  • Keep the compatibility suite out of normal builds and the generic suite matrix.

How are these changes tested?

  • Clean compatibility-suite runs against Celeborn 0.6.0 and 0.7.0 (4 tests each).
  • Clean profile-free test compilation with Spark 3.5 and Scala 2.12, confirming the isolated suite is not compiled normally.
  • Spotless formatting check.
  • python3 dev/ci/check-suites.py.
  • Workflow YAML parsing and git diff --check.

@ziting-openai ziting-openai left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Activate the compatibility profile with a selected Celeborn version.
  2. Compile the isolated suite against the released shaded client.
  3. Resolve and assert the required fields, methods, and version-specific contracts.
  4. 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 sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sunchao
sunchao merged commit 1e10eed into apache:main Sep 1, 2026
79 of 80 checks passed
@sunchao

sunchao commented Sep 1, 2026

Copy link
Copy Markdown
Member

Merged, thanks @pingzh !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Native Celeborn shuffle: add a Celeborn test dependency so the reflection against its internals is verified

3 participants