[GitHub] [spark] dongjoon-hyun commented on a change in pull request #29906: [SPARK-32037][CORE] Rename blacklisting feature

[GitBox <gi...@apache.org>]

dongjoon-hyun commented on a change in pull request #29906:
URL: https://github.com/apache/spark/pull/29906#discussion_r506781069



##########
File path: core/src/main/scala/org/apache/spark/scheduler/HealthTracker.scala
##########
@@ -0,0 +1,478 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.scheduler
+
+import java.util.concurrent.atomic.AtomicReference
+
+import scala.collection.mutable.{ArrayBuffer, HashMap, HashSet}
+
+import org.apache.spark.{ExecutorAllocationClient, SparkConf, SparkContext}
+import org.apache.spark.internal.Logging
+import org.apache.spark.internal.config
+import org.apache.spark.util.{Clock, SystemClock, Utils}
+
+/**
+ * HealthTracker is designed to track problematic executors and nodes.  It supports excluding
+ * executors and nodes across an entire application (with a periodic expiry). TaskSetManagers add
+ * additional logic for exclusion of executors and nodes for individual tasks and stages which
+ * works in concert with the logic here.
+ *
+ * The tracker needs to deal with a variety of workloads, eg.:
+ *
+ *  * bad user code -- this may lead to many task failures, but that should not count against
+ *      individual executors
+ *  * many small stages -- this may prevent a bad executor for having many failures within one
+ *      stage, but still many failures over the entire application
+ *  * "flaky" executors -- they don't fail every task, but are still faulty enough to merit
+ *      excluding
+ *
+ * See the design doc on SPARK-8425 for a more in-depth discussion.
+ *
+ * THREADING: As with most helpers of TaskSchedulerImpl, this is not thread-safe.  Though it is
+ * called by multiple threads, callers must already have a lock on the TaskSchedulerImpl.  The
+ * one exception is [[excludedNodeList()]], which can be called without holding a lock.
+ */
+private[scheduler] class HealthTracker (
+    private val listenerBus: LiveListenerBus,
+    conf: SparkConf,
+    allocationClient: Option[ExecutorAllocationClient],
+    clock: Clock = new SystemClock()) extends Logging {
+
+  def this(sc: SparkContext, allocationClient: Option[ExecutorAllocationClient]) = {
+    this(sc.listenerBus, sc.conf, allocationClient)
+  }
+
+  HealthTracker.validateExcludeOnFailureConfs(conf)
+  private val MAX_FAILURES_PER_EXEC = conf.get(config.MAX_FAILURES_PER_EXEC)
+  private val MAX_FAILED_EXEC_PER_NODE = conf.get(config.MAX_FAILED_EXEC_PER_NODE)
+  val EXCLUDE_ON_FAILURE_TIMEOUT_MILLIS = HealthTracker.getExludeOnFailureTimeout(conf)
+  private val EXCLUDE_FETCH_FAILURE_ENABLED =
+    conf.get(config.EXCLUDE_ON_FAILURE_FETCH_FAILURE_ENABLED)
+
+  /**
+   * A map from executorId to information on task failures. Tracks the time of each task failure,
+   * so that we can avoid excluding executors due to failures that are very far apart. We do not
+   * actively remove from this as soon as tasks hit their timeouts, to avoid the time it would take
+   * to do so. But it will not grow too large, because as soon as an executor gets too many
+   * failures, we exclude the executor and remove its entry here.
+   */
+  private val executorIdToFailureList = new HashMap[String, ExecutorFailureList]()
+  val executorIdToExcludedStatus = new HashMap[String, ExcludedExecutor]()
+  val nodeIdToExcludedExpiryTime = new HashMap[String, Long]()
+  /**
+   * An immutable copy of the set of nodes that are currently excluded.  Kept in an
+   * AtomicReference to make [[excludedNodeList()]] thread-safe.
+   */
+  private val _excludedNodeList = new AtomicReference[Set[String]](Set())
+  /**
+   * Time when the next excluded node will expire.  Used as a shortcut to
+   * avoid iterating over all entries in the excludedNodeList when none will have expired.
+   */
+  var nextExpiryTime: Long = Long.MaxValue
+  /**
+   * Mapping from nodes to all of the executors that have been excluded on that node. We do *not*
+   * remove from this when executors are removed from spark, so we can track when we get multiple
+   * successive excluded executors on one node.  Nonetheless, it will not grow too large because
+   * there cannot be many excluded executors on one node, before we stop requesting more
+   * executors on that node, and we clean up the list of exluded executors once an executor has
+   * been excluded for EXCLUDE_ON_FAILURE_TIMEOUT_MILLIS.
+   */
+  val nodeToExcludedExecs = new HashMap[String, HashSet[String]]()
+
+  /**
+   * Include executors and nodes that have been excluded for at least
+   * EXCLUDE_ON_FAILURE_TIMEOUT_MILLIS
+   */
+  def applyExcludeOnFailureTimeout(): Unit = {
+    val now = clock.getTimeMillis()
+    // quickly check if we've got anything to expire that is excluded -- if not,
+    // avoid doing any work
+    if (now > nextExpiryTime) {
+      // Apply the timeout to excluded nodes and executors
+      val execsToInclude = executorIdToExcludedStatus.filter(_._2.expiryTime < now).keys
+      if (execsToInclude.nonEmpty) {
+        // Include any executors that have been exluded longer than the excludeOnFailure timeout.
+        logInfo(s"Removing executors $execsToInclude from exclude list because the " +
+          s"the executors have reached the timed out")
+        execsToInclude.foreach { exec =>
+          val status = executorIdToExcludedStatus.remove(exec).get
+          val failedExecsOnNode = nodeToExcludedExecs(status.node)
+          listenerBus.post(SparkListenerExecutorUnexcluded(now, exec))
+          failedExecsOnNode.remove(exec)
+          if (failedExecsOnNode.isEmpty) {
+            nodeToExcludedExecs.remove(status.node)
+          }
+        }
+      }
+      val nodesToInclude = nodeIdToExcludedExpiryTime.filter(_._2 < now).keys
+      if (nodesToInclude.nonEmpty) {
+        // Include any nodes that have been excluded longer than the excludeOnFailure timeout.
+        logInfo(s"Removing nodes $nodesToInclude from exclude list because the " +
+          s"nodes have reached has timed out")
+        nodesToInclude.foreach { node =>
+          nodeIdToExcludedExpiryTime.remove(node)
+          listenerBus.post(SparkListenerNodeUnexcluded(now, node))
+        }
+        _excludedNodeList.set(nodeIdToExcludedExpiryTime.keySet.toSet)
+      }
+      updateNextExpiryTime()
+    }
+  }
+
+  private def updateNextExpiryTime(): Unit = {
+    val execMinExpiry = if (executorIdToExcludedStatus.nonEmpty) {
+      executorIdToExcludedStatus.map{_._2.expiryTime}.min
+    } else {
+      Long.MaxValue
+    }
+    val nodeMinExpiry = if (nodeIdToExcludedExpiryTime.nonEmpty) {
+      nodeIdToExcludedExpiryTime.values.min
+    } else {
+      Long.MaxValue
+    }
+    nextExpiryTime = math.min(execMinExpiry, nodeMinExpiry)
+  }
+
+  private def killExecutor(exec: String, msg: String): Unit = {
+    allocationClient match {
+      case Some(a) =>
+        logInfo(msg)
+        a.killExecutors(Seq(exec), adjustTargetNumExecutors = false, countFailures = false,
+          force = true)
+      case None =>
+        logInfo(s"Not attempting to kill excluded executor id $exec " +
+          s"since allocation client is not defined.")
+    }
+  }
+
+  private def killExcludedExecutor(exec: String): Unit = {
+    if (conf.get(config.EXCLUDE_ON_FAILURE_KILL_ENABLED)) {
+      killExecutor(exec, s"Killing excluded executor id $exec since " +
+        s"${config.EXCLUDE_ON_FAILURE_KILL_ENABLED.key} is set.")
+    }
+  }
+
+  private[scheduler] def killExcludedIdleExecutor(exec: String): Unit = {
+    killExecutor(exec,
+      s"Killing excluded idle executor id $exec because of task unschedulability and trying " +
+        "to acquire a new executor.")
+  }
+
+  private def killExecutorsOnExcludedNode(node: String): Unit = {
+    if (conf.get(config.EXCLUDE_ON_FAILURE_KILL_ENABLED)) {
+      allocationClient match {
+        case Some(a) =>
+          logInfo(s"Killing all executors on excluded host $node " +
+            s"since ${config.EXCLUDE_ON_FAILURE_KILL_ENABLED.key} is set.")
+          if (a.killExecutorsOnHost(node) == false) {
+            logError(s"Killing executors on node $node failed.")
+          }
+        case None =>
+          logWarning(s"Not attempting to kill executors on excluded host $node " +
+            s"since allocation client is not defined.")
+      }
+    }
+  }
+
+  def updateExcludedForFetchFailure(host: String, exec: String): Unit = {
+    if (EXCLUDE_FETCH_FAILURE_ENABLED) {
+      // If we exclude on fetch failures, we are implicitly saying that we believe the failure is
+      // non-transient, and can't be recovered from (even if this is the first fetch failure,
+      // stage is retried after just one failure, so we don't always get a chance to collect
+      // multiple fetch failures).
+      // If the external shuffle-service is on, then every other executor on this node would
+      // be suffering from the same issue, so we should exclude (and potentially kill) all
+      // of them immediately.
+
+      val now = clock.getTimeMillis()
+      val expiryTimeForNewExcludes = now + EXCLUDE_ON_FAILURE_TIMEOUT_MILLIS
+
+      if (conf.get(config.SHUFFLE_SERVICE_ENABLED)) {
+        if (!nodeIdToExcludedExpiryTime.contains(host)) {
+          logInfo(s"excluding node $host due to fetch failure of external shuffle service")
+
+          nodeIdToExcludedExpiryTime.put(host, expiryTimeForNewExcludes)
+          listenerBus.post(SparkListenerNodeExcluded(now, host, 1))
+          _excludedNodeList.set(nodeIdToExcludedExpiryTime.keySet.toSet)
+          killExecutorsOnExcludedNode(host)
+          updateNextExpiryTime()
+        }
+      } else if (!executorIdToExcludedStatus.contains(exec)) {
+        logInfo(s"Excluding executor $exec due to fetch failure")
+
+        executorIdToExcludedStatus.put(exec, ExcludedExecutor(host, expiryTimeForNewExcludes))
+        // We hardcoded number of failure tasks to 1 for fetch failure, because there's no
+        // reattempt for such failure.
+        listenerBus.post(SparkListenerExecutorExcluded(now, exec, 1))

Review comment:
       cc @gatorsmile 




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org