Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/2751#issuecomment-58760669
Hi @shijinkui, can you provide some motivation / context for this change?
The scheduler is a complicated piece of code and even simple changes like this
can have unanticipated consequences.
There are two types of Schedulables: pools and TaskSetManagers. It looks
like `TaskSetManager.checkSpeculatableTasks()` has the side-effect of updating
some internal data-structures in TaskSetManager (such as the
`speculatableTasks` HashSet), so your PR's early-termination means that only
the first TaskSetManager in the pool will have its fields updated. I haven't
checked too closely to see how `checkSpeculatableTasks` is called, but I'm
concerned that this change might either cause us to miss out on scheduling some
speculatable tasks or may result in more calls / loops through
`checkSpeculatableTasks` (meaning that it wouldn't be a huge efficiency gain).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
