fix(celeborn): reject unsafe native push completion tracking - #5665
Conversation
sunchao
left a comment
There was a problem hiding this comment.
Reviewed 6540fa0aa77575e92caa9f846a41a1eb02a4fc82 against 90c1dd96c89895bd9732d64055e4919b81a8635d. No actionable P1/P2 findings.
Design and compatibility
Rejecting unsupported completion tracking before admission registration addresses #5528 without treating push-state counters as proof that payloads are released. The planner uses the same capability decision for ordinary exchanges, collect-limit and top-K, preserving ordinary Spark/Celeborn dependencies on fallback. Actual Celeborn 0.6.0 and 0.7.0 clients have final, non-volatile hook fields and are rejected. This does not certify arbitrary future client implementations solely because their fields are volatile.
Performance
Capability detection is captured per manager, with no new per-row or per-frame reflection loop. Released 0.6.0/0.7.0 clients intentionally lose native shuffle acceleration, including when native mode is requested. That is a documented safety tradeoff. No throughput or memory benchmark was run, and no speedup is claimed.
Abstraction and complexity
Reusing one rejection reason and the existing planner gate keeps the decision consistent without introducing a new completion framework. The injected capability supplier supports testing the manager boundary. A safe upstream completion API remains the route to restoring native support for released clients.
Validation and CI
The focused review passed 53 selected pusher tests, five compatibility tests against each released JAR, and 12 field-shape rejection checks. I independently reran the 12 admission checks. The new rejection regression fails on the base and passes on this head. Four Spark-dependent tests were omitted from the local selection. Separately, observed Spark 3.5 merge CI passed 383 tests, including the new AQE-on/off fallback queries.
No full local Spark/JNI run, live Celeborn service or Java 11 runtime/API validation was performed. Local compilation used Java 21 with source/target 11.
CI is not complete: the latest snapshot has 48 successful checks, seven skipped, 16 running and one failed. The TPC-DS verification job stopped while downloading Maven with HTTP 429, before result verification. I did not rerun or approve CI.
|
Merged, thanks @pingzh ! |
Which issue does this PR close?
Closes #5528.
Rationale for this change
Replacing final fields on live Celeborn clients does not guarantee that transport and retry threads observe Comet's ownership hooks. Admission can then be released while payloads remain in flight. Push-state counters alone do not prove that callbacks, retries, and timed-out writes have released those payloads.
What changes are included in this PR?
How are these changes tested?
CelebornShufflePartitionPusherSuite,CometCelebornShuffleManagerSuite, andCometCelebornShufflePlanningSuitein a root reactor build with Spark 4.1.3 / Scala 2.13.17. Coverage includes retained references on another thread and Spark fallback queries with AQE enabled and disabled.CelebornReflectionCompatibilitySuitepassed all 5 tests against each released 0.6.0 and 0.7.0 JAR. These suites were compiled and run separately, including production Java compilation with--release 11.git diff --checkpassed.