[CELEBORN-2415] Add Celeborn Spark UI extension - #3793
Conversation
4f8bad3 to
68fc2f2
Compare
7a9e389 to
47f3676
Compare
47f3676 to
e053012
Compare
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds a new Spark UI / History Server extension module that surfaces Celeborn-related shuffle summary metrics and spark.celeborn.* configuration in a dedicated “Celeborn” tab.
Changes:
- Introduces new module
client-spark/spark-3-uiwith Spark plugin + History Server plugin, status-store access, and UI rendering. - Adds a Spark 3 vs Spark 4 servlet (
javaxvsjakarta) bridge via profile-selected source roots. - Wires the new UI module into root build and shaded Spark client artifacts.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| pom.xml | Adds the new UI module to Spark build profiles and introduces servlet.source.dir to select Spark3/Spark4 servlet bridge sources. |
| client-spark/spark-3-ui/pom.xml | New module POM with Spark dependency + servlet APIs and build-helper source-root selection. |
| client-spark/spark-4-shaded/pom.xml | Includes the new UI artifact in the Spark 4 shaded assembly dependencies. |
| client-spark/spark-3-shaded/pom.xml | Includes the new UI artifact in the Spark 3 shaded assembly dependencies. |
| client-spark/spark-3-ui/src/main/scala/org/apache/spark/shuffle/celeborn/CelebornPlugin.scala | Adds SparkPlugin entrypoint for live UI, registers listener and attaches tab. |
| client-spark/spark-3-ui/src/main/scala/org/apache/spark/shuffle/celeborn/CelebornHistoryServerPlugin.scala | Adds SHS plugin SPI implementation to replay metrics and attach the tab. |
| client-spark/spark-3-ui/src/main/scala/org/apache/spark/shuffle/celeborn/CelebornListener.scala | Listener aggregates task metrics and persists rollups + properties into KVStore. |
| client-spark/spark-3-ui/src/main/scala/org/apache/spark/shuffle/celeborn/CelebornStatusStore.scala | Status-store accessors for aggregated metrics and celeborn properties. |
| client-spark/spark-3-ui/src/main/scala/org/apache/spark/shuffle/celeborn/ui/CelebornUITab.scala | Defines the “Celeborn” tab and registers the page. |
| client-spark/spark-3-ui/src/main/scala/org/apache/spark/shuffle/celeborn/ui/CelebornShufflePage.scala | Renders summary metrics + collapsible properties table. |
| client-spark/spark-3-ui/src/main/scala-spark3/org/apache/spark/shuffle/celeborn/ui/SparkServletBridge.scala | Spark 3 servlet type alias (javax.servlet). |
| client-spark/spark-3-ui/src/main/scala-spark4/org/apache/spark/shuffle/celeborn/ui/SparkServletBridge.scala | Spark 4 servlet type alias (jakarta.servlet). |
| client-spark/spark-3-ui/src/main/resources/META-INF/services/org.apache.spark.status.AppHistoryServerPlugin | Registers the SHS plugin via SPI. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| private[celeborn] class CelebornPropertiesUIData( | ||
| val info: Seq[(String, String)]) { | ||
|
|
||
| @JsonIgnore | ||
| @KVIndex | ||
| def id: String = classOf[CelebornPropertiesUIData].getName | ||
| } |
There was a problem hiding this comment.
Not necessary. Verified end-to-end that the properties table renders correctly in both live UI and SHS replay. Spark's KVStore uses Jackson with the Scala
| if (statusStore.hasData()) { | ||
| new CelebornUITab(statusStore, ui) | ||
| } |
There was a problem hiding this comment.
Fair point on the live/SHS inconsistency. Fixed by attaching the tab unconditionally in SHS (matching the live UI, which always attaches once spark.plugins=CelebornPlugin is set) and removing the now-unused hasData().
| private val totalTaskDurationMs = new AtomicLong(0L) | ||
|
|
||
| private val lastUpdateTimestamp = new AtomicLong(-1L) | ||
| private val updateIntervalMillis = 5000L |
There was a problem hiding this comment.
Reasonable for later, but intentionally a fixed default for the MVP. We can expose spark.celeborn.ui.flushIntervalMs in a follow-up once we see real cadence
needs.
| val last = lastUpdateTimestamp.get() | ||
| if (!force && (last != -1L && (now - last) < updateIntervalMillis)) { | ||
| return | ||
| } | ||
| if (lastUpdateTimestamp.compareAndSet(last, now) || force) { | ||
| flushAggregations() | ||
| } |
There was a problem hiding this comment.
Impact is negligible in practice: force=true is only reached from onApplicationEnd, which fires once at app end with no concurrent onTaskEnd. No extra flushes follow it. Leaving as-is.
| <dependency> | ||
| <groupId>jakarta.servlet</groupId> | ||
| <artifactId>jakarta.servlet-api</artifactId> | ||
| <scope>provided</scope> | ||
| </dependency> |
There was a problem hiding this comment.
Intentional. A single module pom with ${servlet.source.dir} selecting the source root keeps the build simple; both servlet APIs are provided and only the
one matching the selected source root is ever referenced at compile time. Splitting into per-profile dependencies would duplicate the dependency block
across every Spark profile without real benefit.
| <module>client-spark/spark-3</module> | ||
| <module>client-spark/spark-3-columnar-common</module> | ||
| <module>client-spark/spark-3-columnar-shuffle</module> | ||
| <module>client-spark/spark-3-ui</module> |
There was a problem hiding this comment.
The name follows the existing celeborn-client-spark-3 convention in this repo, which also serves Spark 4. Renaming would diverge from that. Keeping spark-3-ui for consistency.
| <span class="collapse-aggregated-celebornProperties collapse-table" | ||
| onClick="collapseTable('collapse-aggregated-celebornProperties', | ||
| 'aggregated-celebornProperties')"> | ||
| <h4> | ||
| <span class="collapse-table-arrow arrow-open"></span> | ||
| <a>Celeborn Properties</a> | ||
| </h4> | ||
| </span> | ||
| <div class="aggregated-celebornProperties collapsible-table"> |
There was a problem hiding this comment.
Following Spark's own UI convention here — Spark uses camelCase collapse identifiers (e.g. collapse-aggregated-allActiveStages). The collapseTable() JS
accepts arbitrary ids as long as the collapse-* prefix and the target class match, which they do. No change.
setupUI() no longer gates on hasData(); the Celeborn tab is now always attached in the History Server, matching the live UI behavior where the tab is attached unconditionally once spark.plugins=CelebornPlugin is set. Removes the now-unused CelebornStatusStore.hasData().
c0cd150 to
18cf1d1
Compare
|
ping @Kalvin2077 |
|
Could you also add |
@Kalvin2077 Thanks for the review! Done. |
|
@pan3793 @cxzl25 @RexXiong |
|
Two issues to address: 1. Flush accumulated metrics when processing finishes The 5-second interval in The same issue affects History Server replay of logs without an Could we flush at job completion (or periodically) for the live UI, and register an 2. Honor the application's plugin opt-in in the History Server
Could we gate metric collection and tab creation on whether the application's recorded |
…play close; honor plugin opt-in in SHS - Flush per-job via onJobEnd so task metrics landing inside the throttle interval are persisted for long-lived idle applications (live UI). - Register an ElementTrackingStore.onFlush callback so replay persists the final accumulated values even without an ApplicationEnd event. - During replay, gate metric collection on the application's recorded spark.plugins (from the event log, not the History Server's own conf) and persist the opt-in as a KVStore marker; setupUI attaches the Celeborn tab only when the marker is present. - Add CelebornListenerSuite covering both regressions.
|
@RexXiong Thanks for the careful review! Both issues are addressed:
|
|
Thanks for addressing the flush behavior and the History Server opt-in! One remaining issue: the new Could you add |
…ntUi in SBT build
|
@RexXiong Added |
| if (!pluginEnabled) { | ||
| val pluginClass = classOf[CelebornPlugin].getName | ||
| val optedIn = sparkProps.exists { case (k, v) => | ||
| k == "spark.plugins" && v.split(",").exists(_.trim == pluginClass) |
There was a problem hiding this comment.
Do we need to consider the spark.plugins.defaultList configuration item?
What changes were proposed in this pull request?
Add a new module
celeborn-client-spark-3-ui— a Spark UI extension that renders a "Celeborn" tab in both the live Spark UI and the History Server (enabled viaspark.plugins=org.apache.spark.shuffle.celeborn.CelebornPlugin, supports Spark 3.x and 4.x).This is an initial version that lays the foundation; it intentionally starts minimal. The page shows:
TaskMetricsspark.celeborn.*configsThe skeleton is:
CelebornPlugin(live UI) /CelebornHistoryServerPlugin(SHS, viaAppHistoryServerPluginSPI) → listener aggregates into SparkKVStore→CelebornStatusStore→CelebornUITabrenders; ajavax/jakartaservlet bridge covers Spark 3.x and 4.x. This plugin/store/render pipeline is designed so that fine-grained Celeborn client metrics can be added as collapsible sections in follow-up PRs:Why are the changes needed?
Spark UI already shows stage/task-level shuffle metrics, but nothing
Celeborn-specific: which workers served the data, push/chunk latencies, congestion and split events are invisible there, while Celeborn's own metrics sit in cluster-level Grafana dashboards that cannot be attributed back to a specific application. This tab closes that gap by putting per-application Celeborn shuffle information inside the Spark UI and History Server, alongside the job's stages and tasks.Does this PR introduce any user-facing change?
Yes.
Setting
spark.plugins=org.apache.spark.shuffle.celeborn.CelebornPlugin(with the extension jar on the classpath) adds a "Celeborn" tab to the Spark UI and History Server. Disabled by default; no effect otherwise.Does this PR resolve a correctness bug?
No.
How was this patch tested?
Manual end-to-end verification: