-
Notifications
You must be signed in to change notification settings - Fork 28k
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-2608] fix executor backend launch commond over mesos mode #2103
Conversation
@pwendell take a look at the new fix |
QA tests have started for PR 2103 at commit
|
QA tests have finished for PR 2103 at commit
|
what's the diff with my pr #1986? |
My last commit is the diff in this pr |
Btw this is still not ideal IMO since it computes the class path in the scheduler side and assumes all slave executors after unzip has the same setup. |
@@ -29,9 +28,12 @@ import org.apache.mesos.{Scheduler => MScheduler} | |||
import org.apache.mesos._ | |||
import org.apache.mesos.Protos.{TaskInfo => MesosTaskInfo, TaskState => MesosTaskState, _} | |||
|
|||
import org.apache.spark.{Logging, SparkContext, SparkException, TaskState} | |||
import org.apache.spark.{SparkConf, Logging, SparkContext, TaskState, SparkException} |
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.
May be you could organize your imports using this guide
Hey it is not clear how you are fixing the stated bug. It would be great if you could explain or somehow exclude the "code cleanups" from the PR. |
offer.getHostname, numCores)) | ||
command.addUris(CommandInfo.URI.newBuilder().setValue(uri)) | ||
mesosCommand.setValue(CommandUtils.buildCommandSeq(command, sc.executorMemory, | ||
sparkHome).mkString("\"", "\" \"", "\"")) |
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 don't think sparkHome
works for the uri != null
branch. Since executors inherit their spark.home
property from the driver, but in case of Mesos, each executor has it's own Spark home in its own sandbox directory. That's why basename
and the cd %s*
trick is used 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.
I see, looks like I get the intention. I was wondering what are other changes about ?
@andrewor14 np! |
based on @scwf patch, rebased on master and have a fix to actually get it to work.
It failed to run with a single mesos master/slave without the fix.