dongjoon-hyun commented on code in PR #58965:
URL: https://github.com/apache/spark/pull/58965#discussion_r4459563140
##########
core/src/main/scala/org/apache/spark/deploy/history/HistoryServer.scala:
##########
@@ -163,6 +165,38 @@ class HistoryServer(
attachHandler(contextHandler)
}
+ /**
+ * Registers the static resources declared by every available plugin. A
plugin that cannot be
+ * loaded, or whose declared resources cannot be registered, is skipped with
a warning so that
+ * it does not prevent the History Server from starting. Throwable is caught
around both
+ * hasNext and next() because the ServiceLoader iterator can throw
ServiceConfigurationError
+ * or LinkageError for a provider whose class is missing or cannot be linked.
+ */
+ private[history] def setupPluginStaticResources(
+ plugins: Iterable[AppHistoryServerPlugin]): Unit = {
+ val iterator = plugins.iterator
+ var keepLoading = true
+ while (keepLoading) {
+ try {
+ if (iterator.hasNext) {
+ val plugin = iterator.next()
+ try {
+ plugin.staticResources.foreach { case (dir, path) =>
addStaticHandler(dir, path) }
+ } catch {
+ case t: Throwable =>
+ logWarning(log"Failed to set up static resources for " +
Review Comment:
**[11] When one declared entry fails, the earlier entries stay, the later
ones are skipped, and the warning names none of them.**
One `try` covers the whole `foreach` at L184. Say a plugin declares three
entries and the directory of the second is missing. The first entry stays
registered, the third is never tried, and this warning says only that the
plugin's static resources failed, without a directory or a path. The pages that
need the third directory then get 404s, and nothing in the log names that
directory. Could the `try` go inside the `foreach`, with the warning naming the
`(dir, path)` pair? `SQLHistoryServerPlugin` declares only one entry, so this
is only about the contract.
##########
core/src/main/scala/org/apache/spark/deploy/history/HistoryServer.scala:
##########
@@ -163,6 +165,38 @@ class HistoryServer(
attachHandler(contextHandler)
}
+ /**
+ * Registers the static resources declared by every available plugin. A
plugin that cannot be
+ * loaded, or whose declared resources cannot be registered, is skipped with
a warning so that
+ * it does not prevent the History Server from starting. Throwable is caught
around both
+ * hasNext and next() because the ServiceLoader iterator can throw
ServiceConfigurationError
+ * or LinkageError for a provider whose class is missing or cannot be linked.
+ */
+ private[history] def setupPluginStaticResources(
+ plugins: Iterable[AppHistoryServerPlugin]): Unit = {
+ val iterator = plugins.iterator
+ var keepLoading = true
+ while (keepLoading) {
+ try {
+ if (iterator.hasNext) {
+ val plugin = iterator.next()
+ try {
+ plugin.staticResources.foreach { case (dir, path) =>
addStaticHandler(dir, path) }
Review Comment:
**[10] The declared directory is looked up with Spark's class loader, not
the plugin's.**
`loadPlugins()` finds plugins through `Utils.getContextOrSparkClassLoader`
(AppHistoryServerPlugin.scala:63). `addStaticHandler`, however, resolves `dir`
in `JettyUtils.createStaticHandler`, which uses
`Utils.getSparkClassLoader.getResource` (JettyUtils.scala:247). If only the
context class loader can see a plugin, `ServiceLoader` finds the plugin, but
`createStaticHandler` throws "Could not find resource path for Web UI". L186
then logs it, and the files are not served. The new doc
(AppHistoryServerPlugin.scala:45) describes them as "JS/CSS bundled with the
plugin", which suggests the plugin's own class loader.
This does not affect `SQLHistoryServerPlugin`, whose directory is on Spark's
class path. Could the doc say that the directory must be on Spark's class path?
Alternatively, the lookup could use `plugin.getClass.getClassLoader`.
##########
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)
+ setupPluginStaticResources(AppHistoryServerPlugin.loadPlugins())
Review Comment:
**[12] A declared path can take over the History Server's own routes.**
Plugin handlers are attached here, before the `/history` loader handler
(L162-165), and their paths are not checked. Jetty picks the longest matching
context path. Of two handlers with the same path, it picks the one attached
first, which I checked with Jetty 12.1.13. So:
- a plugin that declares `/history` answers every application URL instead of
`loaderServlet`;
- one that declares `/history/x` or `/api/v1/x` takes those requests;
- one that declares `/static` is ignored without any message.
Plugins are trusted code, so this is about catching a mistake early. Could
`setupPluginStaticResources` reject a path that is not under `/static/`, with
the same warning?
##########
sql/core/src/test/scala/org/apache/spark/deploy/history/SQLHistoryServerStaticResourceSuite.scala:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.deploy.history
+
+import java.net.URI
+
+import org.apache.spark.{SecurityManager, SparkConf, SparkFunSuite}
+import org.apache.spark.internal.config.History.HISTORY_LOG_DIR
+import org.apache.spark.util.Utils
+
+/**
+ * Verifies that the SQL plugin's static resources are served by the History
Server itself, so
+ * they are available before any application UI has been loaded (and stay
available after an
Review Comment:
**[14] The test covers neither the eviction this doc mentions nor the
precedence the fix relies on.**
The doc says the files "stay available after an application UI is evicted
from the cache". The test, however, never loads or evicts an application; it
checks only an empty log directory. The fix and the ACL note in the description
also rely on the History Server's own handler answering before the
`/static/sql` handlers that `SQLTab` and `StreamingQueryTab` attach for each
loaded application. That holds today. With Jetty 12.1.13, of two context
handlers with the same path, the one attached first answers, even after the
other is removed and a new one is added. But this is Jetty's tie-break, and
nothing here would notice if it changed.
Could a case write an event log with one SQL execution, load its UI, detach
it, and then request `/static/sql/spark-sql-viz.js` again? To skip the cache,
it could call `server.attachSparkUI` and `server.detachSparkUI` with the UI
from `provider.getAppUI`. A request made while the UI is attached would also
show which handler answers. If that is too much for this PR, could the doc
describe only what the test checks?
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/ui/SQLTab.scala:
##########
@@ -34,11 +34,20 @@ class SQLTab(val sqlStore: SQLAppStatusStore, sparkUI:
SparkUI)
attachPage(new ExecutionPage(this))
parent.attachTab(this)
- parent.addStaticHandler(SQLTab.STATIC_RESOURCE_DIR, "/static/sql")
+ SQLTab.addStaticHandler(parent)
Review Comment:
**[18] Nit: in the History Server, the per-application `/static/sql`
handlers can no longer receive a request.**
The History Server's own handler is attached first and always answers, so in
the History Server the handler that this line adds for each loaded application
is now unreachable. The same goes for the one at StreamingQueryTab.scala:35.
Still, each load creates that handler and adds its filters, plus the
cache-check filter for an incomplete application
(ApplicationCache.scala:193-196). It also wraps and starts the handler, and
rebuilds the root handler collection on attach and again on detach
(HistoryServer.scala:218-220 and 226). That cost is small, but it grows with
the cache churn this PR is about. The live UI still needs this line, so one
option is for `HistoryServer.attachSparkUI` to skip any handler whose context
path the server already serves. This is optional.
##########
core/src/main/scala/org/apache/spark/deploy/history/HistoryServer.scala:
##########
@@ -163,6 +165,38 @@ class HistoryServer(
attachHandler(contextHandler)
}
+ /**
+ * Registers the static resources declared by every available plugin. A
plugin that cannot be
+ * loaded, or whose declared resources cannot be registered, is skipped with
a warning so that
+ * it does not prevent the History Server from starting. Throwable is caught
around both
+ * hasNext and next() because the ServiceLoader iterator can throw
ServiceConfigurationError
+ * or LinkageError for a provider whose class is missing or cannot be linked.
+ */
+ private[history] def setupPluginStaticResources(
+ plugins: Iterable[AppHistoryServerPlugin]): Unit = {
+ val iterator = plugins.iterator
+ var keepLoading = true
+ while (keepLoading) {
+ try {
+ if (iterator.hasNext) {
Review Comment:
**[9] The loop never ends when the lookup fails without moving on.**
After any failure, the loop calls `hasNext` again and relies on the iterator
having moved past the bad entry. The JDK `ServiceLoader` does not always move.
In `LazyClassPathLookupIterator.nextProviderClass`, if the class loader's
`getResources` for
`META-INF/services/org.apache.spark.status.AppHistoryServerPlugin` throws an
`IOException`, `configs` stays `null`. `hasNext` keeps the error and returns
`true`, and `next()` throws it and clears it. The next `hasNext` calls
`getResources` again with the same result. So `new HistoryServer(...)` never
returns, and L195 logs the same warning in a tight loop. I ran this loop
against a `ServiceLoader` whose class loader throws from `getResources`, on JDK
21.0.13 and 27.0.1, and it was still looping after 1000 failures. The test
cannot catch this, because `PluginIterator` always moves on.
`HadoopDelegationTokenManager.loadProviders`
(HadoopDelegationTokenManager.scala:339-352), which I pointed to in item 1, has
the same loop, so this comes f
rom my suggestion as much as from the PR.
Could the loop give up after a few failures in a row with no successful
`next()`, and log that the remaining plugins were skipped? For example:
```scala
var failuresInARow = 0
while (keepLoading && failuresInARow < 10) {
try {
if (iterator.hasNext) {
val plugin = iterator.next()
failuresInARow = 0
// ... register plugin.staticResources as now ...
} else {
keepLoading = false
}
} catch {
case t: Throwable =>
failuresInARow += 1
logWarning("Skipping an AppHistoryServerPlugin that failed to
load", t)
}
}
```
While this catch is being changed: `Throwable` also catches
`OutOfMemoryError` and `StackOverflowError`, and reports them as a plugin that
failed to load. Item 1 suggested `Throwable` because `loadProviders` uses it.
`case t if NonFatal(t) || t.isInstanceOf[LinkageError]` would still cover
`ServiceConfigurationError` and `LinkageError`, and would let a fatal error
stop the startup. The same applies to the inner catch at L186. Either choice is
fine with me.
##########
sql/core/src/main/scala/org/apache/spark/sql/streaming/ui/StreamingQueryTab.scala:
##########
@@ -32,11 +32,7 @@ private[sql] class StreamingQueryTab(
attachPage(new StreamingQueryStatisticsPage(this))
parent.attachTab(this)
- parent.addStaticHandler(StreamingQueryTab.STATIC_RESOURCE_DIR, "/static/sql")
+ SQLTab.addStaticHandler(parent)
Review Comment:
**[16] No Structured Streaming page loads anything from `/static/sql`.**
The pages of this tab load only core files: `/static/d3.min.js`,
`/static/streaming-page.css`, `/static/streaming-page.js` and
`/static/structured-streaming-page.js`
(StreamingQueryStatisticsPage.scala:52-55). The directory behind `/static/sql`
holds only SQL files, such as `spark-sql-viz.js` and `executionpage.js`. This
registration has been unused since SPARK-29543 added it. So the description's
"The live driver UI never calls the new hook, so it still needs them" holds for
`SQLTab`, but not for this tab. Keeping the line is fine for this PR. Could the
description say that only `SQLTab` needs it, or note that this one is unused?
##########
core/src/main/scala/org/apache/spark/status/AppHistoryServerPlugin.scala:
##########
@@ -36,8 +41,25 @@ private[spark] trait AppHistoryServerPlugin {
*/
def setupUI(ui: SparkUI): Unit
+ /**
+ * Static resources (JS/CSS bundled with the plugin) to serve from the
History Server's own UI,
+ * as (classpath resource directory, URL path) pairs. They are registered
once at startup.
+ * Unlike a handler attached in [[setupUI]], which is torn down when the
application cache
+ * evicts that application, these live as long as the History Server.
+ *
+ * This does not replace the registration in [[setupUI]] or in the tabs: the
live driver UI
+ * never calls this hook, so tabs must still register their own handlers.
+ */
+ def staticResources: Seq[(String, String)] = Nil
+
/**
* The position of a plugin tab relative to the other plugin tabs in the
history UI.
*/
def displayOrder: Int = Integer.MAX_VALUE
}
+
+private[spark] object AppHistoryServerPlugin {
+ def loadPlugins(): Iterable[AppHistoryServerPlugin] = {
Review Comment:
**[13] Could `loadPlugins()` skip a broken plugin, so that every caller
handles it the same way?**
The new failure handling covers only the static resources.
`FsHistoryProvider.getAppUI` (FsHistoryProvider.scala:450) and
`rebuildAppStore` (FsHistoryProvider.scala:1441-1444) still iterate the raw
`ServiceLoader` view returned here. So a plugin that
HistoryServer.scala:193-195 skips with a warning at startup still throws
`ServiceConfigurationError` out of every application UI load. The description
says this. Now that the loop exists, though, moving it here would let one place
decide.
For example, `loadPlugins()` could use the loop from
`setupPluginStaticResources` and return a `Seq` of the plugins that loaded.
`setupPluginStaticResources` would then be a plain `foreach`, and the test
could check `loadPlugins` instead of a hand-written iterator.
`HadoopDelegationTokenManager.loadProviders`
(HadoopDelegationTokenManager.scala:339-352) has the same loop, so a small
shared helper could serve both.
One caveat: skipping a plugin in `rebuildAppStore` may not be wanted. A
store kept on disk without that plugin's listeners would stay incomplete after
the plugin is fixed. If so, the helper could still serve `HistoryServer` and
`getAppUI`, and `rebuildAppStore` could keep failing fast.
##########
sql/core/src/test/scala/org/apache/spark/deploy/history/SQLHistoryServerStaticResourceSuite.scala:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.deploy.history
+
+import java.net.URI
+
+import org.apache.spark.{SecurityManager, SparkConf, SparkFunSuite}
+import org.apache.spark.internal.config.History.HISTORY_LOG_DIR
+import org.apache.spark.util.Utils
+
+/**
+ * Verifies that the SQL plugin's static resources are served by the History
Server itself, so
+ * they are available before any application UI has been loaded (and stay
available after an
+ * application UI is evicted from the cache).
+ */
+class SQLHistoryServerStaticResourceSuite extends SparkFunSuite {
+
+ test("/static/sql resources are served before any application is loaded") {
+ withTempDir { logDir =>
+ val conf = new SparkConf().set(HISTORY_LOG_DIR, logDir.getAbsolutePath)
+ val provider = new FsHistoryProvider(conf)
+ val server = new HistoryServer(conf, provider, new
SecurityManager(conf), 0)
+ try {
+ server.bind()
+ provider.start()
+ val host = Utils.localHostNameForURI()
+ val url = new
URI(s"http://$host:${server.boundPort}/static/sql/spark-sql-viz.js";).toURL
+ val (code, body, _) = HistoryServerSuite.getContentAndCode(url)
+ assert(code === 200)
+ assert(body.exists(_.nonEmpty))
+ } finally {
+ server.stop()
+ provider.stop()
Review Comment:
**[15] Nit: `provider.stop()` runs twice.**
`HistoryServer.stop()` already calls `provider.stop()`
(HistoryServer.scala:208), so this line stops the provider a second time: its
executors are shut down again and `listing` is closed again. That is harmless
with the in-memory store. Still, the existing suites call only `server.stop()`
(HistoryServerPageSuite.scala:64 and RealBrowserUIHistoryServerSuite.scala:98).
Could this line and HistoryServerPluginSuite.scala:105 go?
--
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]
