dongjoon-hyun commented on code in PR #58346:
URL: https://github.com/apache/spark/pull/58346#discussion_r3873929638
##########
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:
You are right -- the split changed what the config should gate: in a
display-only PR it was hiding data, not a button. Took your one-line fix: the
conjunct is gone, the raw `supported` is reported, and the scaladoc paragraph
and the doc sentence went with it. The `holdEnabled=false` test is kept with
its assertions flipped, as `SPARK-59055: spark.ui.holdEnabled=false does not
suppress the hold status`. The control-side gate stays in the follow-up PR,
which rejects hold requests at the driver when the config is off.
##########
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:
Fixed -- took the suggestion.
##########
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:
Fixed -- added the `isHeld` / `stateText === "WAITING (held)"` assertions
and the singular `draining 1 executor` case (which also let the finished block
drop its own `addExecutor`).
--
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]
