dongjoon-hyun commented on code in PR #58965:
URL: https://github.com/apache/spark/pull/58965#discussion_r4429216332
##########
core/src/main/scala/org/apache/spark/status/AppHistoryServerPlugin.scala:
##########
@@ -36,8 +41,26 @@ private[spark] trait AppHistoryServerPlugin {
*/
def setupUI(ui: SparkUI): Unit
+ /**
+ * Registers static resource handlers (JS/CSS bundled with the plugin)
directly on the
+ * History Server's own UI, rather than on a per-application SparkUI.
Invoked exactly once,
+ * when the History Server starts up.
+ *
+ * A handler attached via [[setupUI]] lives on that one application's
SparkUI and is torn
Review Comment:
**[4] This reads as if registering here replaces the handler attached in
`setupUI`, but `SQLTab` still needs its own.**
The live driver UI never calls this hook, so SQLTab.scala:37 must keep
registering `/static/sql` for it. A later cleanup that follows "Registering it
here instead" and deletes that line would leave the path unserved on the live
UI of every application that never creates a `StreamingQueryManager`: the only
other registration, StreamingQueryTab.scala:35, is built only from the lazy
`SharedState.streamingQueryStatusListener` (SharedState.scala:135-145). No test
would catch it. In the History Server, on the other hand, those per-application
copies no longer serve anything, because the handler registered here is
attached first and answers every `/static/sql` request.
Could the doc add a sentence such as "Tabs still register the same handler
on each SparkUI because the live driver UI never calls this hook; in the
History Server, the handler registered here keeps the path served after those
per-application copies are detached"? Alternatively, `SQLTab` and
`StreamingQueryTab` could register theirs only when `sparkUI.sc.isDefined`, the
way ExecutorsTab.scala:33-35 gates live-only features;
FsHistoryProvider.scala:445 is the only `SparkUI.create(None, ...)` in main
code.
##########
core/src/main/scala/org/apache/spark/deploy/history/HistoryServer.scala:
##########
@@ -155,6 +156,7 @@ class HistoryServer(
attachHandler(ApiRootResource.getServletHandler(this))
addStaticHandler(SparkUI.STATIC_RESOURCE_DIR)
+ AppHistoryServerPlugin.loadPlugins().foreach(_.setupStaticResources(this))
Review Comment:
**[1] A plugin that fails to load now stops the History Server from
starting.**
This line runs inside the constructor (`initialize()` at L144), before
`bind()` in `main` (L320-321). If a jar on the classpath lists an
`AppHistoryServerPlugin` provider whose class is missing or cannot be linked,
or whose constructor throws, the `ServiceLoader` iterator throws
`ServiceConfigurationError` (or a raw `LinkageError` from `hasNext`) out of
`new HistoryServer(...)`, and the daemon exits. Before this PR, only
`FsHistoryProvider.getAppUI` and `rebuildAppStore` loaded plugins
(FsHistoryProvider.scala:450 and 1436), so the same plugin broke only the
application UIs, while the listing page, `/api/v1/applications` and event log
downloads kept working.
A `NonFatal` guard around `setupStaticResources` alone would not be enough,
because these errors come from the iterator, and `NonFatal` does not match
`LinkageError`. Could this call skip a failing plugin with a warning, catching
`Throwable` around both `hasNext` and `next()` as
`HadoopDelegationTokenManager.loadProviders` does
(HadoopDelegationTokenManager.scala:339-357)? If failing fast is intended,
could the PR description say so instead?
##########
core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala:
##########
@@ -1713,9 +1713,7 @@ private[history] class FsHistoryProvider(conf: SparkConf,
clock: Clock)
store
}
- private def loadPlugins(): Iterable[AppHistoryServerPlugin] = {
- ServiceLoader.load(classOf[AppHistoryServerPlugin],
Utils.getContextOrSparkClassLoader).asScala
- }
+ private def loadPlugins(): Iterable[AppHistoryServerPlugin] =
AppHistoryServerPlugin.loadPlugins()
Review Comment:
**[7] Nit: this is now only an alias of
`AppHistoryServerPlugin.loadPlugins()`.**
Could L450 and L1436 call `AppHistoryServerPlugin.loadPlugins()` directly,
as HistoryServer.scala:159 does, and this method go? `AppHistoryServerPlugin`
already resolves through the `org.apache.spark.status._` import (L51), and L450
stays within 100 characters. With one name for the loader, a later change such
as caching cannot land on only one of the two call paths.
##########
core/src/main/scala/org/apache/spark/deploy/history/HistoryServer.scala:
##########
@@ -155,6 +156,7 @@ class HistoryServer(
attachHandler(ApiRootResource.getServletHandler(this))
addStaticHandler(SparkUI.STATIC_RESOURCE_DIR)
+ AppHistoryServerPlugin.loadPlugins().foreach(_.setupStaticResources(this))
Review Comment:
**[2] No test covers the new handler, and the description does not say why.**
The HistoryServer suites in core cannot exercise this line: core's test
classpath has no `AppHistoryServerPlugin` services file, so
`SQLHistoryServerPlugin` is never loaded there. Removing this line or the
override in SQLHistoryServerPlugin.scala:37-39, or changing the mount path,
would bring the 404s back without failing any test.
sql/core already has test code in `org.apache.spark.deploy.history`
(`Utils.scala`), which reaches the `private[history]` classes, and it depends
on core's test-jar. Could you add a small suite there that starts a
`HistoryServer` over an empty log directory on port 0, as
HistoryServerSuite.scala:480 does, and checks that `GET
/static/sql/spark-sql-viz.js` returns 200 before any application is loaded,
e.g. with `HistoryServerSuite.getContentAndCode(url)`? Without this line, the
request falls through to the core `/static` handler, whose directory has no
`sql/` subdirectory, and returns 404.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/ui/SQLHistoryServerPlugin.scala:
##########
@@ -34,6 +34,10 @@ class SQLHistoryServerPlugin extends AppHistoryServerPlugin {
}
}
+ override def setupStaticResources(ui: WebUI): Unit = {
+ ui.addStaticHandler(SQLTab.STATIC_RESOURCE_DIR, "/static/sql")
Review Comment:
**[8] Nit: this line is indented by 6 spaces.**
The other method bodies in this class use 4 (L27 and L31-34). Could you
indent it by 4?
##########
core/src/main/scala/org/apache/spark/status/AppHistoryServerPlugin.scala:
##########
@@ -36,8 +41,26 @@ private[spark] trait AppHistoryServerPlugin {
*/
def setupUI(ui: SparkUI): Unit
+ /**
+ * Registers static resource handlers (JS/CSS bundled with the plugin)
directly on the
+ * History Server's own UI, rather than on a per-application SparkUI.
Invoked exactly once,
+ * when the History Server starts up.
+ *
+ * A handler attached via [[setupUI]] lives on that one application's
SparkUI and is torn
+ * down whenever the application cache evicts it, even though the underlying
Jetty context
+ * path and resources are shared by every application on that server.
Registering it here
+ * instead ties its lifetime to the History Server process itself.
+ */
+ def setupStaticResources(ui: WebUI): Unit = {}
Review Comment:
**[5] Could the hook declare its static resources instead of receiving the
whole `HistoryServer`?**
`setupStaticResources(ui: WebUI)` hands every plugin the History Server
itself, before `bind()`, with `attachPage`, `attachTab`, `attachHandler`,
`detachHandler(path)`, and even `bind()` and `stop()`, although the only
override adds one static handler. The trait is `private[spark]` and plugins are
trusted code, so this is about the contract rather than a bug: from inside the
constructor, a plugin could detach the server's own `/static` or `/api`
handler, or attach pages to its root, which is more than the name and doc
promise. A declarative member in the style of `displayOrder` would do the same
job:
```scala
def staticResources: Seq[(String, String)] = Nil
```
with HistoryServer.scala:159 as
```scala
AppHistoryServerPlugin.loadPlugins().flatMap(_.staticResources).foreach {
case (dir, path) => addStaticHandler(dir, path)
}
```
and in `SQLHistoryServerPlugin`
```scala
override def staticResources: Seq[(String, String)] =
Seq(SQLTab.STATIC_RESOURCE_DIR -> "/static/sql")
```
The handler is then added at the same point and in the same order, the
server keeps control of its own handlers, and "Invoked exactly once" (L46) is
no longer needed.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/ui/SQLHistoryServerPlugin.scala:
##########
@@ -34,6 +34,10 @@ class SQLHistoryServerPlugin extends AppHistoryServerPlugin {
}
}
+ override def setupStaticResources(ui: WebUI): Unit = {
+ ui.addStaticHandler(SQLTab.STATIC_RESOURCE_DIR, "/static/sql")
Review Comment:
**[6] Nit: this types the `/static/sql` mount path once more.**
SQLTab.scala:37 has the same literal, and StreamingQueryTab.scala:35 has a
third copy. `StreamingQueryTab` also keeps its own copy of the directory
(StreamingQueryTab.scala:41), since the `private[ui]` constant at
SQLTab.scala:43 is not visible from `org.apache.spark.sql.streaming.ui`. If the
path later changes in `SQLTab` and in the pages that link to it
(ExecutionPage.scala, AllExecutionsPage.scala) but not here, the 404s come back
only on servers with no application loaded and after evictions, which is easy
to miss. Could object `SQLTab` own the mount in one place, e.g. `private[sql]
def addStaticHandler(ui: WebUI): Unit =
ui.addStaticHandler(STATIC_RESOURCE_DIR, "/static/sql")`, used by
SQLTab.scala:37, this override and StreamingQueryTab.scala:35 (or, with [5],
one shared `(dir, path)` pair)? `STATIC_RESOURCE_DIR` could then stay private.
--
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]
