[GitHub] spark pull request #18492: [SPARK-19326] Speculated task attempts do not get...

[Ngone51 <gi...@git.apache.org>]

Github user Ngone51 commented on a diff in the pull request:

    https://github.com/apache/spark/pull/18492#discussion_r216597619
  
    --- Diff: core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala ---
    @@ -373,8 +373,14 @@ private[spark] class ExecutorAllocationManager(
         // If our target has not changed, do not send a message
         // to the cluster manager and reset our exponential growth
         if (delta == 0) {
    -      numExecutorsToAdd = 1
    -      return 0
    +      // Check if there is any speculative jobs pending
    +      if (listener.pendingTasks == 0 && listener.pendingSpeculativeTasks > 0) {
    +        numExecutorsTarget =
    +          math.max(math.min(maxNumExecutorsNeeded + 1, maxNumExecutors), minNumExecutors)
    --- End diff --
    
    Also confused by `+1` here. And I think we have already task `pendingSpeculativeTasks` into account @advancedxy :
    
    ```
    def totalPendingTasks(): Int = {
      pendingTasks + pendingSpeculativeTasks
    }
    ```
    Seems this check is redundant.
    And it doesn't sync to CM if `numExecutorsTarget ` change(after `+1`).


---

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