-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-12832][MESOS] mesos scheduler respect agent attributes #10949
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
Conversation
|
ok to test |
|
I'll try it out. The code looks good otherwise, thanks for picking up that PR! |
|
Hi @dragos, I'm not sure how to grant access rights to the CI server, but let me know if there's anything I can do to help out here. Thanks. |
|
The failure is spurious, git failed to check out.
|
|
retest this please |
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.
this will fail scalastyle tests; the line needs to be below 100 chars
|
Also cc @tnachen who wrote this code originally. From the JIRA:
Is this caused by duplicate code somewhere? Can we resolve that, either in this patch or separately? |
|
Test build #50500 has finished for PR 10949 at commit
|
|
@andrewor14 this is the one that should go forward. The first sentence of this PR says:
Unfortunately I can't close #10768. |
|
Regarding sharing code: The logic to check constraints is already shared. The actual resource processing isn't. Maybe there is room to share more logic. I opened SPARK-10444. |
|
Test build #51103 has started for PR 10949 at commit |
|
Pushed changes to address scalastyle test failures. Also, in order to run multiple dispatchers on the same mesos cluster, you should set |
|
jenkins, test this please |
|
Test build #51113 has finished for PR 10949 at commit
|
|
Btw can you add a quick unit test for this? We've added tests before already so should be straightforward to do so. |
|
Good idea about the unit test. I don't think it's too hard to add one along the lines of what's already in |
|
Hey, @atongen will you have time to look into the additional test? |
|
Yes, I'll be able to add tests. Sorry for the delay! |
|
any updates here ? |
|
@atongen this PR dose not merge into spark 1.6.1, I hope it could merge it to some version like 1.6.2. |
|
@tnachen, @dragos : I reviewed the tests introduced by #5563, and from what I can tell they are mainly testing only MesosSchedulerUtils#matchesAttributeRequirements in regard to attribute constraints; which is the only significant thing introduced by this PR. Without further refactoring, testing at the next level up (MesosClusterScheduler#scheduleTasks) would require a functioning MesosClusterPersistenceEngine (not the black hole), and quite a bit of additional scaffolding. Let me know if this is still a requirement. I would like to confirm before putting in the effort. |
|
I think we should add tests and I don't think it requires that much refactoring, if you look at MesosClusterSchedulerSuite you can see the test "can handle multiple roles" already tries a submission and verifies it uses the Offer passed in, we can also test by doing a similiar setup where we have a Offer with attributes and without and verify it's performing the correct logic. Tests is very important and we're looking to really increase our coverage as it's getting harder and harder to catch things. |
|
Ok, I'll look into it further, thanks! |
|
Test build #53852 has finished for PR 10949 at commit
|
|
@atongen can you please rebase? The tests look good, but I'd like to see the test suite passing. |
|
Hey @atongen, I think this is really close to being merged, can you please rebase? |
|
ping @atongen |
82f71bc to
e6001e9
Compare
|
Hello @dragos, again, sorry for all the delays. This branch has been rebased onto master, but I've been having some trouble getting the test suite to run. Please advise. |
|
Test build #58169 has finished for PR 10949 at commit
|
|
@atongen please rebase and try again. |
* mesos scheduler respect agent constraints * reduce line length for scalastyle test * update test suites
e6001e9 to
ce3047d
Compare
|
Test build #60406 has finished for PR 10949 at commit
|
|
@tnachen Tests updated and rebased. Let me know if there's anything else I can do to help out. |
|
any update on this ? It is real pain with driver. As i see patch is ready .. question is about when you can merge ? |
|
Can one of the admins verify this patch? |
|
I am going to close this PR because there doesn't appear to be any interest in getting it merged. It's unfortunate, because it was a nice feature. |
|
I think both @tnachen and I have moved on to the non-Spark world in the meantime. Anyway, neither of us had commit rights. I agree it's a pity to drop it, perhaps @andrewor14 could help. |
We have a similar need to what is proposed in #10768 by @Astralidea and reviewed by @dragos. This pull request implements the suggestion in the comments of that PR.
It takes logic similar to what is found in #5563 for the executors and applies it to the mesos cluster scheduler.
The scheduler will now only accept offers from agents with attributes matching the constraints from the submission.