Clarify how to run native Comet -> Celeborn shuffle end-to-end #6523

Activity

1. github-actions

   added

   requires-triage

   on Oct 1, 2026
2. unikdahal commented on Oct 1, 2026

   

   unikdahal

   on Oct 1, 2026

   ContributorAuthor

   More actions

   @pingzh @sunchao would really appreciate your guidance here since you worked closely on the native Celeborn integration. @andygrove looping you in as well since you had looked into the completion-tracking side.
   I’m mainly trying to understand the intended setup and how the native path was validated, so I can make sure I’m testing it the right way on my side. Thanks a lot!
3. pingzh commented on Oct 2, 2026

   

   pingzh

   on Oct 2, 2026

   Contributor

   More actions

   hi @unikdahal we just had this pr #6373

   Yes, we tested the native writer/reader end-to-end against a real Celeborn cluster, including TPC-H queries with results checked against Spark. This was with our Comet fork and a patched Celeborn 0.6.1 client on Spark 4 / Scala 2.13.

   The catch is that our fork uses different completion tracking. Those runs don’t validate the stricter checks now in Apache main, and I haven’t verified a client that passes those checks.

   I’m not aware of a public Celeborn PR providing the completion API we need across retries, cancellation, and transport buffer release. Our fork still relies on private client internals.

   So for your Spark 3.5 / Celeborn 0.7.0 setup, the fallback is expected. Native shuffle on Apache main still needs compatible completion support. Setting spark.comet.shuffle.mode=native alone won’t enable it.
   For reference, these are the core settings we use:

   spark.plugins=org.apache.spark.CometPlugin
   spark.shuffle.manager=org.apache.spark.sql.comet.execution.shuffle.CometCelebornShuffleManager
   spark.comet.exec.enabled=true
   spark.comet.shuffle.enabled=true
   spark.comet.shuffle.mode=native
   spark.celeborn.client.spark.stageRerun.enabled=true
   

   They need the matching client on both the driver and executors, and won’t address the completion check you’re hitting.
4. unikdahal commented on Oct 2, 2026

   

   unikdahal

   on Oct 2, 2026

   ContributorAuthor

   More actions

   Thanks @pingzh, maintaining a patched Celeborn client on our side is completely fine for us.

   The part I’m still trying to understand is what exactly a compatible client needs to provide. From Apache main I can see the structural checks around the completion-tracking fields, but it’s not clear to me whether satisfying those checks alone is enough, or whether there are additional requirements around retries, cancellation, callback ownership, transport buffer release, lifecycle, etc.

   Right now it feels like there isn’t a fully reproducible path for someone outside the original fork to exercise the native implementation end-to-end without reverse-engineering those assumptions from the Comet internals.

   Would it be possible to share the patched 0.6.1 diff/branch you used, or even just outline the minimum Celeborn-side compatibility contract / set of changes the native path expects?

   I’m happy to maintain a 0.7.x fork and validate it against a real Spark 3.5 cluster. I mainly want to make sure I’m implementing the intended semantics rather than just making the admission check pass.
5. andygrove commented on Oct 2, 2026

   

   andygrove

   on Oct 2, 2026

   Member

   More actions

   I’m not aware of a public Celeborn PR providing the completion API we need across retries, cancellation, and transport buffer release. Our fork still relies on private client internals.

   @pingzh @sunchao Can you help me understand this (I know nothing about Celeborn). Is the Celeborn support in Comet actually usable with stock OSS Celeborn?
6. unikdahal commented on Oct 2, 2026

   

   unikdahal

   on Oct 2, 2026

   ContributorAuthor

   More actions

   @andygrove from what I understand, there isn’t currently a way to run the native Comet -> Celeborn shuffle with a stock released OSS Celeborn client.

   With current 0.6.x / 0.7.x releases, Comet can still work with Celeborn through the existing Spark/Celeborn shuffle path, but the new native writer/reader path is rejected by the completion-tracking compatibility checks.

   I think the part that would help most is having a clear guide or compatibility contract describing what a Celeborn client needs to provide for the native path to be considered supported and correct end-to-end.

   @pingzh @sunchao, is that a fair understanding?
7. pingzh commented on Oct 2, 2026

   

   pingzh

   on Oct 2, 2026

   Last edited by pingzh

   Contributor

   More actions

   @andygrove @unikdahal
   I did a round of code comparison between our Comet fork and OSS Comet. Our fork of Comet uses native shuffle path since our version of Comet does not have this commit #5665, so our Comet fork does not fallback, not because of the forked Celeborn client, although we do have a patch to the Celeborn client for more efficient memory management.

   let me see how i can solve it.
8. andygrove

   added

   enhancementNew feature or requestNew feature or request

   area:shuffleShuffle (JVM and native)Shuffle (JVM and native)

   and removed

   requires-triage

   on Oct 5, 2026
9. unikdahal commented on Oct 6, 2026

   

   unikdahal

   on Oct 6, 2026

   ContributorAuthor

   More actions

   @andygrove @pingzh @sunchao I've been digging into why native Celeborn shuffle no longer works with any released client, and I wanted to share my analysis and ask whether we could unblock it.

   The cause is #5665. It was the fix for #5528, which raised the concern about how the push-completion tracker added in #5513 works. That tracker reflectively replaces private final fields on live Celeborn objects.

   The underlying concern in #5528 is fair. The Java memory model doesn't guarantee other threads see those writes, and depending on another project's private internals is fragile in any case. JEP 500 is also moving the JDK toward rejecting this kind of final-field mutation. So I agree this shouldn't be the long-term design.

   That said, I think #5665 went further than the risk warranted:

   • It disables the feature for every user. It now requires those four fields to be volatile, and no released Celeborn (0.6.x / 0.7.x) declares them that way. The native path is rejected unconditionally, whatever spark.comet.shuffle.mode is set to. Native Celeborn shuffle effectively can't be used at all today.
   • The practical risk is low. HotSpot doesn't constant-fold final instance fields of ordinary classes by default; that only applies to records, hidden classes and some JDK internals. Without that, a stale read needs very specific JIT hoisting. Native Celeborn shuffle: reflectively replacing Celeborn's final fields can release push admission while payloads are in flight #5528 itself notes that nothing in the tree reproduces it.
   • The worst case is accounting, not data safety. The payloads are GC-managed byte[], so a missed callback can't cause a use-after-free or corrupt shuffle data. The worst outcome is that the executor-wide admission budget is exceeded for a while. Celeborn's own per-worker in-flight limit still applies on top.

   So we traded a theoretical, low-probability over-release of a soft memory budget for losing the whole integration.

   My suggestion:

   1. Revert fix(celeborn): reject unsafe native push completion tracking #5665, so native Celeborn shuffle works again with stock 0.6 / 0.7. If a full revert feels too risky, an opt-in config with the caveat documented would also work for me.
   2. Open a tracking issue for the proper fix: replace the reflection with a supported completion contract from Celeborn.

   I've already prototyped that proper fix. It's a small, client-only Celeborn API that pushes a caller-owned buffer and runs a release callback exactly once, after every send and retry of the batch has finished. With it, Comet doesn't need to touch Celeborn internals at all. As a bonus it removes the extra payload copies tracked in #6654. I validated it end to end against a real Celeborn 0.7.0 cluster, with results matching vanilla Spark. I plan to propose it to Celeborn upstream first, after I'm back from vacation.

   Getting that through Celeborn and into a release will take a while, though. Unblocking the existing path in the meantime would let people actually use and validate the native integration. Happy to help with the revert or the opt-in flag if that's the direction you prefer.

   👍React with 👍1Reacted by Ping Zhang
10. pingzh commented on Oct 6, 2026

    

    pingzh

    on Oct 6, 2026

    Contributor

    More actions

    @unikdahal thanks for the detailed analysis. I had similar thought about reverting that strict check #5665 considering the potential risk and benefit.

    @andygrove @sunchao wdyt