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-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
