andygrove opened a new issue, #6118:
URL: https://github.com/apache/datafusion-comet/issues/6118

   ### What is the problem the feature request solves?
   
   Comet's Celeborn integration landed across #5501, #5513, #5531, #5537, 
#5604, #5627, #5665 and #5668. By surface area it is a first-class integration 
comparable to Iceberg: its own shuffle manager, its own reader and writer, its 
own config namespace, and its own CI suites. Its user-facing documentation is a 
subsection of the tuning guide.
   
   What exists today:
   
   - `docs/source/user-guide/latest/tuning.md`, "Remote Shuffle with Celeborn" 
(lines 393-467). The substance is largely there, but it is written as a 
specification of behavior rather than as a guide for someone setting it up.
   - `docs/source/user-guide/latest/configs.md`, Shuffle section: prose on the 
two `spark.comet.shuffle.rss.*` settings plus their auto-generated table rows.
   
   The gaps:
   
   1. **Not discoverable.** The Integrations toctree in 
`docs/source/user-guide/latest/index.rst` lists Iceberg, Iceberg Writes, S3 
Credential Providers and Kubernetes. Celeborn is absent, so someone evaluating 
whether Comet works with their remote shuffle service has to full-text search 
or happen to scroll the tuning guide.
   
   2. **No setup instructions.** The tuning guide says to "provide a compatible 
Celeborn Spark client matching the application's Spark and Scala versions on 
both the driver and executors", but never names the artifact 
(`org.apache.celeborn:celeborn-client-spark-3-shaded_<scala-binary-version>`), 
how to supply it, or which Celeborn versions Comet is tested against. `pom.xml` 
pins `celeborn.version` at 0.7.0 solely for the reflection-compatibility test, 
and nothing user-facing states that.
   
   3. **The headline compatibility fact is buried.** Released 0.6.0 and 0.7.0 
clients do not provide the push-completion guarantee native shuffle requires, 
so on every released client today an application retains the delegated 
Spark/Celeborn path and the `spark.comet.shuffle.mode=native` recipe is not 
reachable. That is the first thing a prospective user needs to know, and it 
currently appears in the second paragraph of a tuning subsection, after the 
recipe.
   
   4. **No way to verify what happened.** Nothing tells a user how to 
distinguish an exchange that went native-remote from one delegated to ordinary 
Spark/Celeborn shuffle, or one locally materialized after an oversized row 
(#5668). `metrics.md` has no Celeborn or RSS content.
   
   5. **No troubleshooting.** The conditions that silently retain ordinary 
shuffle are real and numerous: `spark.io.encryption.enabled=true`, an effective 
fallback policy of `ALWAYS` or a reached `AUTO` threshold, dynamic allocation 
with both `spark.shuffle.service.enabled` and 
`spark.dynamicAllocation.shuffleTracking.enabled` off, `auto` or `jvm` shuffle 
mode, unsupported child operators, data types or partitioning. They are 
scattered through the prose as constraints rather than presented as symptoms 
with causes.
   
   ### Describe the potential solution
   
   Add `docs/source/user-guide/latest/celeborn.md` and list it in the 
Integrations toctree, modeled on the Iceberg guide. A suggested shape:
   
   - **Support status up front**: which Celeborn versions work with which Comet 
shuffle mode, and what the delegated path does and does not give you on the 
released clients.
   - **Setup**: the client artifact and coordinates, driver and executor 
placement, the shuffle manager setting, the full config recipe, and the fact 
that Celeborn is an optional application dependency Comet does not bundle.
   - **What is accelerated**: AQE reducer coalescing and mapper-range reads are 
supported; Celeborn physical-skew chunk reads are not.
   - **Verifying**: how to confirm from the plan and from metrics which path an 
exchange took.
   - **Troubleshooting**: the retention and fallback conditions as a 
symptom-to-cause table.
   - **Tuning**: `spark.comet.shuffle.rss.maxFrameBytes` and 
`spark.comet.shuffle.rss.maxInFlightBytes`, referring to the tuning guide 
rather than duplicating it.
   
   Then reduce the tuning-guide section to a short pointer, keeping the 
genuinely tuning-specific material there so the two pages do not drift.
   
   ### Additional context
   
   Adding Celeborn-specific rows to `metrics.md` may be worth splitting into a 
separate issue if the metric names need work first.
   


-- 
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]

Reply via email to