Github user kayousterhout commented on a diff in the pull request:
https://github.com/apache/spark/pull/16376#discussion_r94837292
--- Diff:
core/src/main/scala/org/apache/spark/scheduler/TaskSetManager.scala ---
@@ -159,7 +159,12 @@ private[spark] class TaskSetManager(
addPendingTask(i)
}
- // Figure out which locality levels we have in our TaskSet, so we can do
delay scheduling
+ /**
+ * Track the set of locality levels which are valid given the tasks
locality preferences and
+ * the set of currently available executors. This is updated as
executors are added and removed.
+ * This allows a performance optimization, of skipping levels that
aren't relevant (eg., skip
+ * PROCESS_LOCAL if no tasks could be run PROCESS_LOCAL for the current
set of executors).
+ */
var myLocalityLevels = computeValidLocalityLevels()
--- End diff --
(1) does seem like an issue. I also mostly agree for (2), since the logic
of avoiding unnecessarily waiting for delay timeouts is already handled
(separately from the myLocalityLevels calculation)
[here](https://github.com/squito/spark/blob/930c2b7b02360a6ce3f01db5420f3f326daaf282/core/src/main/scala/org/apache/spark/scheduler/TaskSetManager.scala#L548).
My only hesitation is that myLocalityLevels does allow avoiding the delay
timeout in cases where there are tasks have constraints to run on executors
that haven't been granted to the application, so that use case seems like it
might merit keeping the code (also, if you agree, can you update the
myLocalityLevels comment?). In any case I'd do this in a separate PR.
---
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 [email protected] or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
