review feedback

core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala

@@ -336,7 +336,8 @@ private[spark] class ExecutorAllocationManager(
 
       // If the new target has not changed, avoid sending a message to the cluster manager
       if (numExecutorsTarget < oldNumExecutorsTarget) {
-        // We lower the target number of executors but don't actively kill any yet.  We do this
+        // We lower the target number of executors but don't actively kill any yet.  Killing is
+        // controlled separately by an idle timeout.  Its still helpful to reduce the target number
         // in case an executor just happens to get lost (eg., bad hardware, or the cluster manager
         // preempts it) -- in that case, there is no point in trying to immediately  get a new
         // executor, since we couldn't even use it yet.
@@ -462,9 +463,7 @@ private[spark] class ExecutorAllocationManager(
       executorIdsToBeRemoved
     } else {
       // We don't want to change our target number of executors, because we already did that
-      // when the task backlog decreased.  Normally there wouldn't be any tasks running on these
-      // executors, but maybe the scheduler *just* decided to run a task there -- in that case,
-      // we don't want to count those failures.
+      // when the task backlog decreased.
       client.killExecutors(executorIdsToBeRemoved, adjustTargetNumExecutors = false,
         countFailures = false, force = false)
     }