pgandhi999 commented on a change in pull request #24035: [SPARK-27112] : Spark 
Scheduler encounters two independent Deadlocks …
URL: https://github.com/apache/spark/pull/24035#discussion_r264709208
 
 

 ##########
 File path: 
core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala
 ##########
 @@ -622,67 +633,107 @@ class CoarseGrainedSchedulerBackend(scheduler: 
TaskSchedulerImpl, val rpcEnv: Rp
    * @param countFailures if there are tasks running on the executors when 
they are killed, whether
    *                      those failures be counted to task failure limits?
    * @param force whether to force kill busy executors, default false
+   * @param blacklistingOnTaskCompletion whether the executors are being 
killed due to
+   *                                     blacklisting triggered by the task 
completion event
    * @return the ids of the executors acknowledged by the cluster manager to 
be removed.
    */
   final override def killExecutors(
       executorIds: Seq[String],
       adjustTargetNumExecutors: Boolean,
       countFailures: Boolean,
-      force: Boolean): Seq[String] = {
+      force: Boolean,
+    blacklistingOnTaskCompletion: Boolean): Seq[String] = {
     logInfo(s"Requesting to kill executor(s) ${executorIds.mkString(", ")}")
 
-    val response = synchronized {
-      val (knownExecutors, unknownExecutors) = 
executorIds.partition(executorDataMap.contains)
-      unknownExecutors.foreach { id =>
-        logWarning(s"Executor to kill $id does not exist!")
-      }
-
-      // If an executor is already pending to be removed, do not kill it again 
(SPARK-9795)
-      // If this executor is busy, do not kill it unless we are told to force 
kill it (SPARK-9552)
-      val executorsToKill = knownExecutors
-        .filter { id => !executorsPendingToRemove.contains(id) }
-        .filter { id => force || !scheduler.isExecutorBusy(id) }
-      executorsToKill.foreach { id => executorsPendingToRemove(id) = 
!countFailures }
-
-      logInfo(s"Actual list of executor(s) to be killed is 
${executorsToKill.mkString(", ")}")
-
-      // If we do not wish to replace the executors we kill, sync the target 
number of executors
-      // with the cluster manager to avoid allocating new ones. When computing 
the new target,
-      // take into account executors that are pending to be added or removed.
-      val adjustTotalExecutors =
-        if (adjustTargetNumExecutors) {
-          requestedTotalExecutors = math.max(requestedTotalExecutors - 
executorsToKill.size, 0)
-          if (requestedTotalExecutors !=
-              (numExistingExecutors + numPendingExecutors - 
executorsPendingToRemove.size)) {
-            logDebug(
-              s"""killExecutors($executorIds, $adjustTargetNumExecutors, 
$countFailures, $force):
-                 |Executor counts do not match:
-                 |requestedTotalExecutors  = $requestedTotalExecutors
-                 |numExistingExecutors     = $numExistingExecutors
-                 |numPendingExecutors      = $numPendingExecutors
-                 |executorsPendingToRemove = 
${executorsPendingToRemove.size}""".stripMargin)
-          }
-          doRequestTotalExecutors(requestedTotalExecutors)
-        } else {
-          numPendingExecutors += executorsToKill.size
-          Future.successful(true)
+    var response: Future[Seq[String]] = null
+    val idleExecutorIds = executorIds.filter { id => 
!scheduler.isExecutorBusy(id) }
+    if (!blacklistingOnTaskCompletion) {
 
 Review comment:
   @attilapiros Believe me, making `executorIdToRunningTaskIds` a TrieMap did 
come to my mind and I did give it a shot. It works for our purposes, but we 
realized something else. The above map is of type <String, HashSet>. This map 
might protect the reference to the HashSet object, however, it gives no 
guarantee that the HashSet will not be mutated while accessing the map. It 
basically does not guarantee protection on the HashSet. It would have been much 
easier to implement the TrieMap had the value of the map would have simply been 
an Object and not a collection. I could be wrong here, so if you feel 
otherwise, some pointers would be helpful. Thank you.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
[email protected]


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to