dongjoon-hyun commented on code in PR #58346:
URL: https://github.com/apache/spark/pull/58346#discussion_r3873336199
##########
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:
Good catch, and thank you for the probe. Fixed as you suggested: added
`ApplicationInfo.isHeld` (`held && holdSupported && !isFinished`) and made both
`numDrainingExecutors` and the JSON `held` field read it, so the page and the
endpoint can no longer drift apart. Also added the rendered-page test to
`ReadOnlyMasterWebUISuite`, including the `holdSupported = false` case
asserting the row is not annotated.
##########
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:
Fixed via the same `isHeld` accessor: `held` now writes `obj.isHeld` and
`draining` derives from it, so a finished application reports `held: false,
draining: 0` on both surfaces. Pinned in `MasterSuite` with a `markFinished`
case (while the Master still keeps the executors) -- `ApplicationState` is
`private[master]`, so the finished case is asserted there rather than in
`JsonProtocolSuite`.
##########
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:
Thank you for the ready-made tests -- added both to
`StandaloneDynamicAllocationSuite`. I extended the `spark.ui.holdEnabled=false`
one to call `sc.holdExecutors()` first and wait for `held = true` to arrive at
the Master before asserting `!holdSupported`, so it cannot pass vacuously
before the report lands. Agreed on leaving the `MasterChanged` re-send to a
follow-up.
##########
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:
Fixed -- `reportExecutorHoldStatus()` is now `synchronized`, covering both
transition call sites and the constructor one.
--
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]
