-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-11714][Mesos] Make Spark on Mesos honor port restrictions on coarse grain mode #11157
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
|
add to whitelist |
|
@skonto can you add a link in the description to your other PR? |
|
Done @andrewor14. I will resolve conflicts. |
|
Jenkins test this please |
Conflicts: core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/CoarseMesosSchedulerBackend.scala core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerUtils.scala core/src/test/scala/org/apache/spark/scheduler/cluster/mesos/CoarseMesosSchedulerBackendSuite.scala
|
Test build #51096 has started for PR 11157 at commit |
|
Test build #51095 has finished for PR 11157 at commit
|
|
jenkins, test this please |
|
Test build #51108 has finished for PR 11157 at commit
|
|
Jenkins, test this please |
|
Test build #51123 has finished for PR 11157 at commit
|
|
Need to fix the test. Multiple executors per slave need different handling. |
|
Still working on this had to get aligned with @mgummelt. |
| val (remainingMemResources, memResourcesToUse) = | ||
| partitionResources(afterCPUResources.asJava, "mem", taskMemory) | ||
| val (resourcesLeft, portResourcesToUse) = remainingMemResources | ||
| .partition {r => ! ( r.getType == Value.Type.RANGES & r.getName == "ports" )} |
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 think there is still style problems here.
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.
yes havent finished it.... port resources need to be managed as cpus eg. keep global logistics... but what exactly is the problem for you?
|
Can you fix the whitespacing in general in your patch? There are quite some extra whitespaces in the Utils.scala |
|
ok i will fix it thnx... WIP |
|
updated the PR @tnachen @mgummelt, @dragos ready for review.... At offer evaluation step check (scheduler side) if we have enough ports to start an executor in an offer:
Our proposed solution is to start one executor if the user sets a value to spark.executor.port since there is only one port available with that value. Spark Cluster framework port honoring will be fixed in a new PR, to make things easier and shorter for review. |
|
Test build #52982 has finished for PR 11157 at commit
|
33d74aa to
731c2f0
Compare
|
Test build #52983 has finished for PR 11157 at commit
|
|
Test build #52984 has finished for PR 11157 at commit
|
731c2f0 to
023eedc
Compare
|
Test build #53002 has finished for PR 11157 at commit
|
023eedc to
c6ceb8b
Compare
|
Test build #53007 has finished for PR 11157 at commit
|
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.
Can we get a test where the offer contains the requested ports? We should both that the offer is accepted, and that the launched task contains the requested port as a resource.
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.
ok
edf9320 to
b46b9d4
Compare
|
@mgummelt added the extra test needed and fixed the rest. |
|
Test build #63559 has finished for PR 11157 at commit
|
|
@mgummelt : ready for merge what do you think? |
|
LGTM. @srowen Can we get this merged into master? |
| : (List[Resource], List[Resource]) = { | ||
| if (requestedPorts.isEmpty) { | ||
| (offeredResources, List[Resource]()) | ||
| } |
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.
Can we do a quick minor style pass, since that's all I can comment on really -- pull this else up to the previous line for consistency.
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.
ok
|
@srowen ready |
|
Test build #63755 has finished for PR 11157 at commit
|
|
@srowen could you merge this pls? |
|
Merged to master |
|
woot |
|
has this change be documented to spark on mesos guide? |
|
@sun-rui Will do in a separate pr shortly. There was a question for that a few comments above. Thanx for mention it again. |
Previous attempt was for fine grained: #10808