-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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
[SPARK-5095] [Mesos] Support launching multiple mesos executors in coarse grained mesos mode. #10993
Conversation
@@ -89,13 +82,11 @@ private[spark] class CoarseMesosSchedulerBackend( | |||
*/ | |||
private[mesos] def executorLimit: Int = executorLimitOption.getOrElse(Int.MaxValue) | |||
|
|||
private val pendingRemovedSlaveIds = new HashSet[String] |
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.
AFAICT, this is never used, so I removed it.
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.
nice
Test build #50419 has finished for PR 10993 at commit
|
val taskIdToSlaveId: HashBiMap[Int, String] = HashBiMap.create[Int, String] | ||
// How many times tasks on each slave failed | ||
val failuresBySlaveId: HashMap[String, Int] = new HashMap[String, Int] | ||
private val slaves = new HashMap[String, Slave] |
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.
Not a big issue, but I wonder if slaveInfo
would be a better name. Also, a one-line comment to explain what the key is.
Looks like this is failing real tests |
Also, have you had a chance to test this with cluster mode and/or with dynamic allocation? |
f421133
to
318486e
Compare
I made an update that should fix the test. I've tested in cluster mode, but not with dynamic allocation. Though I have added some unit tests that cover dynamic allocation. I'll see about setting up dynamic allocation. |
Test build #50506 has finished for PR 10993 at commit
|
I didn't have time to look at this in detail, I'll do so this afternoon. |
val id = offer.getId.getValue | ||
|
||
if (tasks.contains(offer.getId)) { // accept | ||
val filters = Filters.newBuilder().setRefuseSeconds(5).build() |
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 know this is not your code, but it would be good to document this. Why do we filter out offers for 5 seconds on the offers we use?
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 was thinking about this when I ran into it. The default is actually 5: https://github.com/apache/mesos/blob/master/include/mesos/mesos.proto#L1211
So I'll just remove it.
@mgummelt this looks really good! I have a few comments. I still have to run this PR with dynamic allocation and see it in action! |
318486e
to
0a1181a
Compare
Test build #50608 has finished for PR 10993 at commit
|
@mgummelt If I want to scheduler deploy with : Limiting a slave can only start one executor, how to do that? should I can have another config like spark.executor.maxperslave=1? |
* @param offers Mesos offers that match attribute constraints | ||
* @return A map from OfferID to a list of Mesos tasks to launch on that offer | ||
*/ | ||
private def getMesosTasks(offers: Buffer[Offer]): Map[OfferID, List[MesosTaskInfo]] = { |
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 find it non-productive to quibble over a name. That being said, this method doesn't just get tasks from somewhere. It produces them itself, based on a round-robin scheduling strategy over the given offers. I don't think get
is the best verb to describe that action.
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.
Sure. Changed to build
@andrewor14 Glad to be here! Flaky tests or no I think all concerns have been addressed except for dynamic allocation testing, which seems to be broken entirely: SPARK-12583 @dragos Any other comments? |
Test build #50833 has finished for PR 10993 at commit
|
LGTM! Great work, @mgummelt! |
val slaveId: String = status.getSlaveId.getValue | ||
val taskId = status.getTaskId.getValue | ||
val slaveId = status.getSlaveId.getValue | ||
val slave = slaves(slaveId) |
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 needs to be moved in the stateLock right?
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.
Moved, but I don't understand why the mesos methods are synchronized in the first place. They should only be called by a single thread (the driver thread).
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.
The ExecutionAllocationManager
, used when dynamic allocation is enabled, runs on a different thread. Not sure if this particular method can run on different threads, but there's at least the issue of visibility.
Just one comment, overall LGTM |
Support spark.executor.cores on Mesos.
7e3f39d
to
ecad77a
Compare
retest this please |
Test build #50985 has finished for PR 10993 at commit
|
Test build #50984 has finished for PR 10993 at commit
|
// SlaveID -> Slave | ||
// This map accumulates entries for the duration of the job. Slaves are never deleted, because | ||
// we need to maintain e.g. failure state and connection state. | ||
private val slaves = new HashMap[String, Slave] |
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.
elsewhere in Spark we would call this class SlaveInfo
instead of just Slave
, so we don't confuse it with the Mesos Slave
LGTM merging into master. @mgummelt feel free to address the remainder of the comments in a follow-up patch. |
Thanks for merging. Will this go into 1.6.1, or not until 2.0? |
This is a big new feature. It will not go into a maintenance release (1.6.1). |
@mgummelt looks like this caused a flaky test: Do you have the bandwidth to fix it quickly? If not I'll just revert this patch for now and we can resubmit it later. |
looking into it |
@mgummelt Great Work! I think this feature will allow more people to use mesos. |
@andrewor14 I haven't found the problem, but here's a PR to remove the test in the interim #11164 It's a strange test to be flaky. It's very simple. |
Ah, I see the issue. There's a thread causing a race. I won't be able to fix until tomorrow, though. |
…rse grained mesos mode. This is the next iteration of tnachen's previous PR: apache#4027 In that PR, we resolved with andrewor14 and pwendell to implement the Mesos scheduler's support of `spark.executor.cores` to be consistent with YARN and Standalone. This PR implements that resolution. This PR implements two high-level features. These two features are co-dependent, so they're implemented both here: - Mesos support for spark.executor.cores - Multiple executors per slave We at Mesosphere have been working with Typesafe on a Spark/Mesos integration test suite: https://github.com/typesafehub/mesos-spark-integration-tests, which passes for this PR. The contribution is my original work and I license the work to the project under the project's open source license. Author: Michael Gummelt <mgummelt@mesosphere.io> Closes apache#10993 from mgummelt/executor_sizing.
…rse grained mesos mode. This is the next iteration of tnachen's previous PR: apache#4027 In that PR, we resolved with andrewor14 and pwendell to implement the Mesos scheduler's support of `spark.executor.cores` to be consistent with YARN and Standalone. This PR implements that resolution. This PR implements two high-level features. These two features are co-dependent, so they're implemented both here: - Mesos support for spark.executor.cores - Multiple executors per slave We at Mesosphere have been working with Typesafe on a Spark/Mesos integration test suite: https://github.com/typesafehub/mesos-spark-integration-tests, which passes for this PR. The contribution is my original work and I license the work to the project under the project's open source license. Author: Michael Gummelt <mgummelt@mesosphere.io> Closes apache#10993 from mgummelt/executor_sizing.
…rse grained mesos mode. This is the next iteration of tnachen's previous PR: apache#4027 In that PR, we resolved with andrewor14 and pwendell to implement the Mesos scheduler's support of `spark.executor.cores` to be consistent with YARN and Standalone. This PR implements that resolution. This PR implements two high-level features. These two features are co-dependent, so they're implemented both here: - Mesos support for spark.executor.cores - Multiple executors per slave We at Mesosphere have been working with Typesafe on a Spark/Mesos integration test suite: https://github.com/typesafehub/mesos-spark-integration-tests, which passes for this PR. The contribution is my original work and I license the work to the project under the project's open source license. Author: Michael Gummelt <mgummelt@mesosphere.io> Closes apache#10993 from mgummelt/executor_sizing.
This is the next iteration of @tnachen's previous PR: #4027
In that PR, we resolved with @andrewor14 and @pwendell to implement the Mesos scheduler's support of
spark.executor.cores
to be consistent with YARN and Standalone. This PR implements that resolution.This PR implements two high-level features. These two features are co-dependent, so they're implemented both here:
We at Mesosphere have been working with Typesafe on a Spark/Mesos integration test suite: https://github.com/typesafehub/mesos-spark-integration-tests, which passes for this PR.
The contribution is my original work and I license the work to the project under the project's open source license.