peter-toth commented on code in PR #58346:
URL: https://github.com/apache/spark/pull/58346#discussion_r3873834555
##########
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 = {
+ Option(client).foreach(
+ _.reportHoldStatus(supported && conf.get(config.UI.UI_HOLD_ENABLED),
held))
Review Comment:
**Finding 8.** This conjunct now decides whether the Master shows anything
at all, because `isHeld` ANDs `holdSupported` in. But `spark.ui.holdEnabled`
gates a *driver-UI button* — `holdExecutors()` is a `@DeveloperApi` the config
does not touch, and only `AllJobsPage.scala:358` and `JobsTab.scala:109,133`
read it. So an application that turns the button off and drives holds
programmatically gets nothing on the Master, which is the gap this PR opens
with ("An operator on the Master page cannot tell which applications are held
or still draining").
Measured in `StandaloneDynamicAllocationSuite` — `holdEnabled=false`,
`sc.holdExecutors()`, waiting for `held` to reach the Master, 180 ms:
```
executors=2 state=RUNNING held=true holdSupported=false isHeld=false
draining=0 stateText='RUNNING'
{..."state":"RUNNING","holdsupported":false,"held":false,"draining":0,...}
```
Two executors are sitting on the Master, still draining, and `/json/` says
`held: false, draining: 0` — while `JsonProtocol.scala:99` documents `held` as
"whether the application is currently held; always false once it finishes". The
control run (config left at its default) on the same setup gives
`stateText='RUNNING (held, draining 2 executors)'` and
`"held":true,"draining":2`.
This is my finding 1 landing on the wrong side of the two fixes it offered.
Annotating always costs one doc sentence; gating costs real data. One line:
```suggestion
_.reportHoldStatus(supported, held))
```
`holdSupported` then means "this deployment can hold this application",
which is what `/json/`'s `holdsupported` description already claims, and
`isHeld`'s `holdSupported` conjunct survives as a cheap guard —
`holdExecutors()` `require`s the preconditions, so `held` without `supported`
is unreachable once the config is out of it. I applied exactly this and re-ran:
`holdEnabled=false` then gives `holdSupported=true isHeld=true draining=2
stateText='RUNNING (held, draining 2 executors)'`, 45 ms. (It fits on one line
after the edit; `config` stays used elsewhere in the file.)
Two edits go with it: the scaladoc above loses its `spark.ui.holdEnabled`
paragraph, and `docs/spark-standalone.md:777-778` loses "which requires
`spark.ui.holdEnabled` to be true on that application". `SPARK-59055:
spark.ui.holdEnabled=false is reported as not holdable` inverts — it fails with
the change, and is worth keeping with its assertions flipped, as the test that
pins the config *not* suppressing the status.
If you'd rather keep the gate, it needs documenting on the config itself
(`UI.scala:96-102` and `configuration.md:1609`) — nothing today tells an
operator that turning the driver button off also blanks the Master's view — and
`JsonProtocol.scala:99` needs the qualifier on `held`.
##########
core/src/test/scala/org/apache/spark/deploy/master/MasterSuite.scala:
##########
@@ -250,6 +250,35 @@ class MasterSuite extends MasterSuiteBase {
assert(master.invokePrivate(_createApplication(desc, null)).id ===
"spark-45756")
}
+ test("SPARK-59055: The executors of a held application are counted as
draining") {
+ val appInfo = makeAppInfo(1024)
+ val worker = DeployTestUtils.createWorkerInfo()
+ appInfo.addExecutor(worker, 1, 1024, Map.empty,
ResourceProfile.DEFAULT_RESOURCE_PROFILE_ID)
+ appInfo.addExecutor(worker, 1, 1024, Map.empty,
ResourceProfile.DEFAULT_RESOURCE_PROFILE_ID)
+
+ // The executors of an application that is not held are not draining.
+ assert(appInfo.numDrainingExecutors === 0)
+
+ // A hold that the driver did not report as supported is not treated as
held.
+ appInfo.held = true
+ assert(appInfo.numDrainingExecutors === 0)
+
+ // While held, an executor that has not exited yet is still draining its
running tasks.
+ appInfo.holdSupported = true
+ assert(appInfo.numDrainingExecutors === 2)
+
+ // The hold is complete once the last executor is gone.
+ appInfo.executors.values.toSeq.foreach(appInfo.removeExecutor)
+ assert(appInfo.numDrainingExecutors === 0)
Review Comment:
**Finding 10.** Here the application is held with no executors left —
`stateText` renders `WAITING (held)`, the "hold complete" signal the docs point
operators at — but the assertion is `numDrainingExecutors === 0`, which is also
what a not-held application returns (`:260`, `:264`) and what a finished one
returns (`:279`). So nothing in the suite tells "hold complete" apart from "not
held", and `stateText`'s `draining == 0` branch
(`ApplicationInfo.scala:230-231`) has no coverage at all:
```suggestion
assert(appInfo.numDrainingExecutors === 0)
assert(appInfo.isHeld)
assert(appInfo.stateText === "WAITING (held)")
```
The singular `executor` form (`:233`) is unpinned too —
`ReadOnlyMasterWebUISuite` covers only the plural. One `addExecutor` after the
block above gives `WAITING (held, draining 1 executor)`. I ran both additions
against this head, 39 ms.
##########
core/src/main/scala/org/apache/spark/SparkContext.scala:
##########
@@ -715,6 +715,10 @@ class SparkContext(config: SparkConf) extends Logging {
postEnvironmentUpdate()
postApplicationStart()
+ // Advertise whether this application can be held, now that the shuffle
driver components and
+ // the allocation manager, which decide it, are up.
Review Comment:
**Finding 9.** `executorHoldSupported` (`:2095-2102`) reads
`cg.supportsExecutorHold`, `SHUFFLE_SERVICE_ENABLED` /
`shuffleDriverComponents.supportsReliableStorage()` and `DECOMMISSION_ENABLED`.
The allocation manager is not in it, and `_executorsHeld` is still `false` at
this point, so nothing here depends on
`_executorAllocationManager.foreach(_.start())` at `:712` — only on
`_shuffleDriverComponents` at `:659`.
```suggestion
// Advertise whether this application can be held, now that the shuffle
driver components,
// which decide it, are up.
```
--
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]
