unikdahal commented on issue #6523: URL: https://github.com/apache/datafusion-comet/issues/6523#issuecomment-6019598622
@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. #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 #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. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
