Github user kayousterhout commented on the issue:
https://github.com/apache/spark/pull/14079
OK done with a pass over this -- overall this looks good. Sorry for the
huge number of naming suggestions. This behavior is subtle/complex and I'm in
favor of spending some effort to make it as understandable as possible to
future folks who modify this code.
A few more comments:
The design doc mentions some invariants that must be true of the parameters
(e.g., that some values must be lower than others). Can you enforce those
invariants in the creation of the BlacklistManager?
It would be helpful to add a comment somewhere describing the division of
functionality. Maybe a good place would be in the BlacklistTracker docstring?
Something that explains that the TaskSetManager is responsible for handling
blacklisting at the task set and task level, while the TaskSchedulerImpl is
responsible for handling application-level blacklists?
I added a few high level comments / concerns to the design doc, also.
---
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
