agrawaldevesh commented on a change in pull request #29452:
URL: https://github.com/apache/spark/pull/29452#discussion_r476978623
##########
File path:
core/src/test/scala/org/apache/spark/scheduler/TaskSetManagerSuite.scala
##########
@@ -2077,6 +2086,98 @@ class TaskSetManagerSuite
assert(manager.resourceOffer("exec1", "host1", ANY)._1.isEmpty)
}
+ test("SPARK-21040: Tasks are not speculated on decommissioning if
speculation is disabled") {
Review comment:
You are right about the first part:
`TaskSetManager#checkSpeculatableTasks` is only ever called directly (without
TaskSchedulerImpl) in the `TaskSetManagerSuite`, and in all of those calls,
speculation is indeed always enabled.
So I think I can delete the test in the `TaskSetManagerSuite` and the
corresponding change to `TaskSetManager#checkSpeculatableTasks` to check the
`speculationEnabled` flag.
But I would like to retain the test in `TaskSchedulerImplSuite`: I hear that
it may be a redundant test but the newly added test is really quick and IMHO it
never hurts to ensure invariants without relying on a lower level test -- ie
the double testing does no harm and it provides some coverage even if the
underlying behavior changes. So unless you have strong feelings here, how about
I keep the additional test in `TaskSchedulerImplSuite` ?
Thanks !
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
