zahed1994 commented on code in PR #58380:
URL: https://github.com/apache/spark/pull/58380#discussion_r3882940311
##########
core/src/test/scala/org/apache/spark/ExecutorAllocationManagerSuite.scala:
##########
@@ -2065,6 +2065,31 @@ class ExecutorAllocationManagerSuite extends
SparkFunSuite {
private def executorsDecommissioning(manager: ExecutorAllocationManager):
Set[String] = {
manager.executorMonitor.executorsDecommissioning()
}
+
+ test("SPARK-49485: request additional executor when speculative tasks equal
maxNeeded") {
+ val manager = createManager(createConf(1, 5, 2))
+ assert(maxNumExecutorsNeededPerResourceProfile(manager, defaultProfile)
=== 0)
+
+ // Submit stage with 2 tasks
+ post(SparkListenerStageSubmitted(createStageInfo(0, 2)))
+ post(SparkListenerExecutorAdded(0, "executor-1", new ExecutorInfo("host1",
1, Map.empty)))
+ post(SparkListenerExecutorAdded(0, "executor-2", new ExecutorInfo("host1",
1, Map.empty)))
+
+ // Task 0 and Task 1 start on executor-1 and executor-2
+ val taskInfo0 = createTaskInfo(0, 0, "executor-1")
+ val taskInfo1 = createTaskInfo(1, 1, "executor-2")
+ post(SparkListenerTaskStart(0, 0, taskInfo0))
+ post(SparkListenerTaskStart(0, 0, taskInfo1))
+
+ // 2 tasks running, 2 active executors. maxNeeded = 2
+ assert(maxNumExecutorsNeededPerResourceProfile(manager, defaultProfile)
=== 2)
+
+ // Task 0 is submitted as speculatable
+ post(SparkListenerSpeculativeTaskSubmitted(0, 0))
+
+ // With pendingSpeculative > 0 and maxNeeded == activeExecutors (2),
offset allocates 1 more
+ assert(maxNumExecutorsNeededPerResourceProfile(manager, defaultProfile)
=== 3)
Review Comment:
Thanks for the review @sunchao! Good catch , with `tasksPerExecutor = 1`,
`ceil((2 + 1) / 1) = 3` was already returning 3 on master even without the fix.
I have updated the regression test to configure 2-core executors
(`spark.executor.cores = 2`) with 3 running tasks across 2 active executors and
1 pending speculative task (4 total tasks).
Without this fix (base), `maxNeeded` evaluates to `ceil(4 / 2) = 2`. With
this fix (head), since `maxNeeded` (2) equals the active executor count (2) and
`pendingSpeculative > 0`, the new offset triggers and requests `2 + 1 = 3`
executors, properly exercising the new allocation branch.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
