Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/3393#issuecomment-64096717
I went ahead and reviewed this overall function (@andrewor14 merged some
recent changes which @tnachen authored) and there seem to be multiple issues.
Can you guys comment on the following?
1. The variable named `acceptedOffers` - why is it named this way if it
doesn't indicate that the offers were accepted. Should it be
`acceptableOffers`? Or `usableOffers`? When defining that variable it would be
nice to have a comment that explains what is going on. Basically, you are
excluding ahead of time nodes which we know ahead of time don't have enough
memory to launch an executor or enough cores to launch even a single task. That
should be in a comment.
2. Why does mesos need `2 * scheduler.CPUS_PER_TASK` cores for a node to be
considered and not `1 + scheduler.CPUS_PER_TASK`? The executor needs a single
core, it says so right in the comments.
3. Likewise when it subtracts a CPU for the executor, why does it subtract
`scheduler.CPUS_PER_TASK`? Shouldn't it just subtract one?
As for this fix specifically. Right now it uses the absence of a node in
`slaveIdsWithExecutors` to infer that the offer was not accepted. What happens
if an offer is not used that is adding cores to an existing executor. Won't
that offer be lost here and never replied to? It seems like to do this in a
correct way we need to associate the returned task descriptions with resource
offers.
I'm not an expert in this area of the code but a cursory glance at this
functionality reveals some potential issues.
---
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]
