agrawaldevesh commented on a change in pull request #29452:
URL: https://github.com/apache/spark/pull/29452#discussion_r476699231
##########
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:
I think that this test is testing the right things but perhaps this is
not the right place for them. Basically as you observed, it is merely testing
that the flag is working as expected: In effect it is testing that
`TaskSetManager#checkSpeculatableTasks` is returning false when speculation is
disabled, which is quite trivial.
The other thing that it is checking (which is actually not covered very well
directly) is that when a decommissioned executor is finally lost, its tasks are
rerun and not anytime before that. I think while this is tested sort of at a
higher level (in the worker unit tests), it is not directly validated in either
the task set manager nor the task scheduler tests.
I hear you and I think I wasn't thinking very hard when I added the test. I
am going to split it up into two pieces (and thus also reduce the code
duplication):
- I will make this test just ensure that
`TaskSetManager#checkSpeculatableTasks` returns false and that nothing is
speculated when the speculation is disabled. It does not need to bother with
decommissioning as such.
- I will fold in the logic for ensuring that the task is rerun on executor
lost in one of the task scheduler's decommissioning tests.
Btw, on code duplication in tests: I think it should be okay since we really
want each test to be independant to understand. Putting a lot of helper method,
just makes it hard to understand. I think test code should be more biased
towards independence/flexibility instead of repetition, unlike production code.
But I can see it your way as well :-)
Thanks for flagging this @Ngone51 and it helps improve the test code quality
!.
Also cc' @holdenk for her thoughts.
----------------------------------------------------------------
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]
