cloud-fan commented on code in PR #58054:
URL: https://github.com/apache/spark/pull/58054#discussion_r3810012398


##########
core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala:
##########
@@ -175,6 +177,28 @@ private[spark] class ExecutorAllocationManager(
   //   (2) an executor idle timeout has elapsed.
   @volatile private var initializing: Boolean = true
 
+  // Whether allocation is suspended because the executors are held. While 
this is true,
+  // `schedule()` is a no-op so that pending tasks do not bring up new 
executors.
+  // See `SparkContext.holdExecutors()`.
+  private var suspended: Boolean = false
+
+  // Whether the current executor targets still have to be pushed to the 
cluster manager. Set
+  // when a push from `suspend()`/`resume()` fails or is rejected (e.g. before 
the YARN AM has
+  // registered), and by `reset()`, which may run inside a cluster manager RPC 
handler where a
+  // synchronous request would self-deadlock (e.g. YARN's 
RegisterClusterManager). The push is
+  // performed from the allocation thread in `schedule()` and retried until 
acknowledged, with
+  // an exponential backoff: the conditions it retries (an AM restart, an 
unreachable cluster

Review Comment:
   **Nit:**
   
   ```suggestion
     // an exponential backoff: the conditions under which it retries (an AM 
restart, an unreachable
   ```



##########
core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala:
##########
@@ -739,6 +776,41 @@ class CoarseGrainedSchedulerBackend(scheduler: 
TaskSchedulerImpl, val rpcEnv: Rp
     executorDataMap.keySet.toSeq
   }
 
+  /** See `SparkContext.holdExecutors()`. */
+  private[spark] def setExecutorsHeld(held: Boolean): Unit = {
+    executorsHeld = held

Review Comment:
   **Blocking:**
   
   Please enforce the hold at the executor-requirement mutation path, not only 
at registration. While this flag is true, `requestExecutors` and 
`requestTotalExecutors` still publish nonzero targets, so the cluster allocates 
executors just to have this guard drain them; then `resumeExecutors()` restores 
the pre-hold snapshot and loses those requests. Keep the latest desired target 
in the backend, publish zero until resume, and cover both public request APIs 
with hold/request/resume interleaving tests.



-- 
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]

Reply via email to