New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add IndexedSet to help manage workers/jobs. #463
Conversation
This is definitely interesting. Let me think about it a bit more (haven't had a lot of time in the past few days), but it might be worth going for if we can use this throughout. |
This also fixes a bug where a StatusUpdate message after an executor had already been removed would result in a NoSuchElementException when updating freeCores.
Conflicts: core/src/main/scala/spark/scheduler/cluster/StandaloneSchedulerBackend.scala
Yesterday I had a job run into a race condition in StandaloneSchedulerBackend where freeCores was being decremented in StatusUpdate, but the executor had already been removed, so it failed with NoSuchElementException:
I thought this would be a good excuse for more IndexedSet cuteness, so fixed the bug by having just one "executors" map and ensuring the executor still exists before updating it. Also merged in master and ran the tests. |
Conflicts: core/src/main/scala/spark/deploy/master/Master.scala
Again remerged in master with your job->app/split->partition changes (nice!). |
totalCoreCount.addAndGet(cores) | ||
makeOffers() | ||
makeOffers(e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this change is right--we only have 1 new executor, so I believe calling makeOffers(e) is fine, vs. the previous behavior which would re-makeOffers() for the new + all existing executors.
Conflicts: core/src/main/scala/spark/deploy/master/Master.scala
Can one of the admins verify this patch? |
I'm the Jenkins test bot for the UC, Berkeley AMPLab. I've noticed your pull request and will test it once an admin authorizes me to. Thanks for your submission! |
1 similar comment
I'm the Jenkins test bot for the UC, Berkeley AMPLab. I've noticed your pull request and will test it once an admin authorizes me to. Thanks for your submission! |
Thank you for your pull request. An admin will review this request soon. |
This is admittedly somewhat cute, but I like the idea of more strictly/DRYly applying the "remove the job from all xxxToJob indexes" logic, instead of having to manually remember to do the "xxxToJob -= job" calls.