peter-toth commented on code in PR #58346:
URL: https://github.com/apache/spark/pull/58346#discussion_r3873233515
##########
core/src/main/scala/org/apache/spark/deploy/master/ui/MasterPage.scala:
##########
@@ -311,6 +311,24 @@ private[ui] class MasterPage(parent: MasterWebUI) extends
WebUIPage("") {
</tr>
}
+ /**
+ * The application state, annotated with the hold reported by its driver. An
executor that has
+ * not exited yet is still draining its running tasks; the hold is complete
at zero. A finished
+ * application is never annotated: its driver is gone, so the last reported
hold is stale.
+ */
+ private def appStateText(app: ApplicationInfo): String = {
+ if (!app.held || app.isFinished) {
Review Comment:
**Finding 1.** `appStateText` never reads `holdSupported`, so the
`spark.ui.holdEnabled` gate that
`StandaloneSchedulerBackend.reportExecutorHoldStatus` builds has no effect on
the annotation. Two claims go with it that don't hold:
- `docs/spark-standalone.md:722-724` — "Only applications whose driver
reports that it can be held are annotated, which requires
`spark.ui.holdEnabled` to be true on that application".
- the scaladoc on the override — "reports itself as not holdable, so that
the Master does not show a hold status that the driver UI does not offer to
change".
`holdExecutors()` is a `@DeveloperApi` that the config does not gate; only
the driver-UI link at `AllJobsPage.scala:358` and the POST handlers at
`JobsTab.scala:109,133` read it. So an application started with
`spark.ui.holdEnabled=false` that calls `sc.holdExecutors()` reports
`supported=false, held=true`, and the Master page annotates it regardless.
Observed rather than argued — a probe in `ReadOnlyMasterWebUISuite` with
`holdSupported = false`, `held = true` and two executors renders `WAITING
(held, draining 2 executors)` on `GET /`, and `/json/` returns `"holdsupported"
: false, "held" : true, "draining" : 2` (127 ms). `holdSupported` has no reader
outside `JsonProtocol` today, so the field this PR added to make the decision
is unused at the place the decision is made.
Together with finding 2 this collapses into one accessor on
`ApplicationInfo`, which also removes the chance of the page and the endpoint
drifting apart:
```scala
/** Whether the application is held right now, per the last report from its
running driver. */
private[deploy] def isHeld: Boolean = held && holdSupported && !isFinished
private[deploy] def numDrainingExecutors: Int = if (isHeld) executors.size
else 0
```
`appStateText` then opens with `if (!app.isHeld)` and `JsonProtocol` writes
`("held" -> obj.isHeld)`. If you'd rather keep the annotation driven by `held`
alone, the doc sentence and the scaladoc clause both need to drop the claim
instead.
Worth pinning either way, and cheap: `ReadOnlyMasterWebUISuite` already
builds `ApplicationInfo`s by hand and asserts against the rendered HTML, so the
test is roughly
```scala
app1.held = true
app1.holdSupported = true
app1.addExecutor(createWorkerInfo(), 1, 1024, Map.empty,
DEFAULT_RESOURCE_PROFILE_ID)
app1.addExecutor(createWorkerInfo(), 1, 1024, Map.empty,
DEFAULT_RESOURCE_PROFILE_ID)
// GET / ...
assert(result.contains("WAITING (held, draining 2 executors)"))
```
plus the same with `holdSupported = false` asserting the row is *not*
annotated.
##########
core/src/main/scala/org/apache/spark/SparkContext.scala:
##########
@@ -2308,6 +2314,20 @@ class SparkContext(config: SparkConf) extends Logging {
logWarning("Resuming executors is not supported by current scheduler.")
false
}
+ reportExecutorHoldStatus()
+ acknowledged
+ }
+
+ /**
+ * Tell the cluster manager whether this application can be held and whether
it currently is,
+ * so that it can show the hold status on its own UI. Called once the
context is fully started
+ * -- `executorHoldSupported` reads the shuffle driver components, which are
initialized late
+ * -- and again after every transition.
+ */
+ private def reportExecutorHoldStatus(): Unit = schedulerBackend match {
Review Comment:
**Finding 4.** The two callers invoke this after leaving the `synchronized`
block that flipped `_executorsHeld`, so the read here and the `send` it feeds
are not ordered against a concurrent transition. `holdExecutors()` and
`resumeExecutors()` are both reachable from the driver UI POST handlers
(`JobsTab.scala:109,133`), which run on separate Jetty threads, so: the hold
sets the flag and reads `true`; the resume then clears it, reads `false` and
sends first; the hold's `true` arrives last. The Master is then left showing
`(held)` for a running application, and `send` is fire-and-forget, so nothing
corrects it until the next transition.
Reading and sending under the same monitor is enough — whichever call
serializes last reads and reports the current value. `send` only enqueues, so
holding the monitor across it does not block:
```scala
private def reportExecutorHoldStatus(): Unit = synchronized {
schedulerBackend match {
case cg: CoarseGrainedSchedulerBackend =>
cg.reportExecutorHoldStatus(executorHoldSupported, _executorsHeld)
case _ =>
}
}
```
That covers both call sites and the one at `SparkContext.scala:720`, which
is uncontended anyway.
##########
core/src/main/scala/org/apache/spark/deploy/JsonProtocol.scala:
##########
@@ -112,6 +115,9 @@ private[deploy] object JsonProtocol {
.toList.map(writeResourceRequirement)) ~
("submitdate" -> obj.submitDate.toString) ~
("state" -> obj.state.toString) ~
+ ("holdsupported" -> obj.holdSupported) ~
+ ("held" -> obj.held) ~
Review Comment:
**Finding 2.** `held` and `draining` are written unguarded, and
`writeMasterState` runs `writeApplicationInfo` over `completedApps` too
(`JsonProtocol.scala:244`), so a finished application keeps reporting its last
hold. `appStateText` deliberately suppresses exactly this case — "its driver is
gone, so the last reported hold is stale" — and
`docs/spark-standalone.md:719-720` says `/json/` "reports the same in the
`holdsupported`, `held`, and `draining` fields". So the two new surfaces
disagree, and the doc describes neither.
`draining` is the worse half. `Master.scala:563` keeps a finished
application's executors on purpose (`if (!appInfo.isFinished)
appInfo.removeExecutor(exec)`), so a held application that is then killed from
the Master page reports its executor count as draining indefinitely. Measured
with a `MasterSuite` probe — `held = true`, one executor,
`markFinished(FINISHED)` — gives `"held" : true` and `"draining" : 1`.
An operator scripting `/json/` to find held applications gets completed ones
back. Same fix as finding 1: fold `isFinished` into a single accessor on
`ApplicationInfo` and have both the page and the endpoint read it, so neither
can be updated without the other.
##########
core/src/main/scala/org/apache/spark/deploy/master/ui/MasterPage.scala:
##########
@@ -348,7 +366,7 @@ private[ui] class MasterPage(parent: MasterWebUI) extends
WebUIPage("") {
</td>
<td>{UIUtils.formatDate(app.submitDate)}</td>
<td>{app.desc.user}</td>
- <td>{app.state.toString}</td>
+ <td>{appStateText(app)}</td>
Review Comment:
**Finding 5.** The Master's per-application page is not annotated —
`ApplicationPage.scala:92` still renders `<li><strong>State:</strong>
{app.state}</li>`. So an operator who sees `RUNNING (held, draining 2
executors)` here and clicks the app id lands on the page that lists those
executors and finds a bare `RUNNING`. That page is arguably the more natural
place for the count, since `draining` is derived from the very executor table
it shows.
Calling the same helper from `ApplicationPage` would need it somewhere both
pages can reach (`UIUtils`, or a small method on `ApplicationInfo` next to
`numDrainingExecutors`). Fine as a follow-up if you'd rather keep this PR to
the listing page, but the two Master pages disagreeing about the same
application is worth closing.
##########
docs/spark-standalone.md:
##########
@@ -711,6 +711,19 @@ holders for server-side environment variables like the
following.
{% endraw %}
```
+# Monitoring Held Applications
Review Comment:
**Finding 7.** This lands between `## REST API` and `# Resource Scheduling`,
about 60 lines above the existing `# Monitoring and Logging` section (:773) —
which is where the file already describes what the Master web UI shows, and
where a reader looks for this. Moving it there, or making it a `##` under it,
also keeps the launch → resource-scheduling flow unbroken.
##########
core/src/main/scala/org/apache/spark/scheduler/cluster/StandaloneSchedulerBackend.scala:
##########
@@ -252,6 +252,16 @@ private[spark] class StandaloneSchedulerBackend(
// executors can be held gracefully.
private[spark] override def supportsExecutorHold: Boolean = true
+ /**
+ * `spark.ui.holdEnabled` gates the hold and resume controls on the driver
UI. An application
+ * that opted out of being held reports itself as not holdable, so that the
Master does not
+ * show a hold status that the driver UI does not offer to change.
+ */
+ private[spark] override def reportExecutorHoldStatus(supported: Boolean,
held: Boolean): Unit = {
Review Comment:
**Finding 3.** Nothing exercises this override or
`SparkContext.reportExecutorHoldStatus`. `AppClientSuite` calls
`client.reportHoldStatus` directly, so it starts one hop after both new pieces
of driver-side logic: the `schedulerBackend match` dispatch in `SparkContext`
with its three call sites, and the `supported &&
conf.get(config.UI.UI_HOLD_ENABLED)` conjunct here. The conjunct is the one the
docs make a promise about, so it is the one most worth pinning.
`StandaloneDynamicAllocationSuite` is a good home — it already holds a real
`Master` reference and a `getApplications()` helper, and its workers fake
executor launches, so no shuffle service has to run: `executorHoldSupported`
only reads the two confs. I ran this there against the PR head, 187 ms:
```scala
test("SPARK-59055: report the hold status of the application to the Master")
{
sc = new SparkContext(appConf
.set(config.SHUFFLE_SERVICE_ENABLED, true)
.set(config.DECOMMISSION_ENABLED, true))
// The report from the end of the SparkContext constructor.
eventually(timeout(10.seconds), interval(10.millis)) {
assert(getApplications().length === 1)
assert(getApplications().head.holdSupported)
}
assert(!getApplications().head.held)
assert(sc.holdExecutors())
eventually(timeout(10.seconds), interval(10.millis)) {
assert(getApplications().head.held)
}
// The faked executors never exit, so all of them are still counted as
draining.
assert(getApplications().head.numDrainingExecutors === 2)
assert(sc.resumeExecutors())
eventually(timeout(10.seconds), interval(10.millis)) {
assert(!getApplications().head.held)
}
}
```
A second one adding `.set(config.UI.UI_HOLD_ENABLED, false)` and asserting
`!getApplications().head.holdSupported` covers the conjunct (40 ms) — that is
the test that surfaced finding 1.
The failover re-send from `MasterChanged` stays uncovered after this; that
one is genuinely harder and fine as a follow-up.
##########
core/src/main/scala/org/apache/spark/deploy/client/StandaloneAppClient.scala:
##########
@@ -330,6 +346,17 @@ private[spark] class StandaloneAppClient(
}
}
+ /**
+ * Report to the Master whether this application can be held and whether it
currently is, so
+ * that the Master UI can show the hold status of the application. The
status is cached and
+ * re-sent on failover, so a report made before the registration completes
is not lost.
+ */
+ def reportHoldStatus(supported: Boolean, held: Boolean): Unit = {
+ if (endpoint.get != null) {
Review Comment:
**Finding 6.** This returns silently when `endpoint.get` is null, while both
siblings in the class log on the same condition — `requestTotalExecutors` at
:344 and `killExecutors` at :368 ("Attempted to ... before driver fully
initialized"). A dropped hold report leaves the Master's status wrong with
nothing in the log to explain it, and unlike the siblings there is no return
value for the caller to notice. An `else logWarning(...)` in the neighbours'
shape would do.
--
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]
