Skip to content

[SPARK-5088] Use spark-class for running executors directly #3897

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

Closed
wants to merge 5 commits into from

Conversation

jongyoul
Copy link
Member

@jongyoul jongyoul commented Jan 5, 2015

No description provided.

@SparkQA
Copy link

SparkQA commented Jan 5, 2015

Test build #25051 has started for PR 3897 at commit ed906b7.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Jan 5, 2015

Test build #25051 has finished for PR 3897 at commit ed906b7.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • val executorPath = new File(executorSparkHome, s"/bin/spark-class $executorBackendName")
    • command.setValue(s"cd $

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25051/
Test PASSed.

@tnachen
Copy link
Contributor

tnachen commented Jan 5, 2015

What's the motivation for this change?

@jongyoul
Copy link
Member Author

jongyoul commented Jan 6, 2015

@tnachen On the contrary, why mesos has two different launcher? I couldn't find any idea in a history of that codes, and spark-executor calls spark-class which has a case statement internally. I think that merging two similar codes is a good idea in maintaining codes.

@tnachen
Copy link
Contributor

tnachen commented Jan 6, 2015

I'm not part of that history either, I just want to know the motivation since the PR doesn't say anything.
For code cleanup it does make sense.
Your patch looks good to me, unfortunately we need to find some committer to look at this now....

@jongyoul
Copy link
Member Author

jongyoul commented Jan 6, 2015

@tnachen I'm sorry for not having any comment. I think that this PR is very clear to guess my opinion.

@pwendell Please review this PR which makes some mesos codes clean. Because this PR is about maintaining code, I ask you to review this PR.

@jongyoul
Copy link
Member Author

jongyoul commented Jan 9, 2015

@JoshRosen @tgravescs @andrewor14 Could anyone review this PR? That makes mesos codes clean

@jongyoul
Copy link
Member Author

/cc @mateiz How about this patch? I don't know who maintains mesos' codes mainly. Because you are one of committers of mesos, I ask you to review this simple PR, which is about removing redundant script file. I've already tested this PR in my cluster

@jongyoul
Copy link
Member Author

Please review this PR

@mateiz
Copy link
Contributor

mateiz commented Jan 13, 2015

This looks okay, but is there any motivation for it other than getting rid of that source file?

if (uri == null) {
val executorPath = new File(executorSparkHome, "/sbin/spark-executor").getCanonicalPath
val executorPath = new File(executorSparkHome, s"/bin/spark-class $executorBackendName")
.getCanonicalPath
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually weird, why are we including $executorBackendName in the filename? We should just look for spark-class. I'm not even sure getCanonicalPath will work on a file that doesn't exist

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's my mistake. I'll fix it right now. Because I always set spark.executor.uri, That's not tested.

@jongyoul
Copy link
Member Author

@mateiz That's all, but I think it is meaningful to remove some redundant codes and helps understand codes easy. And I wonder why that code exists. I think spark-executoris just a wrapper for launching spark-class. And we need to try to merge coarse and fine grained mode. That's the first and easiest effort for it

@mateiz
Copy link
Contributor

mateiz commented Jan 14, 2015

Coarse and fine-grained mode are different on purpose, I don't think it makes sense to merge those. BTW have you tested both of them on this?

@jongyoul
Copy link
Member Author

@mateiz I've test it both mode.

@SparkQA
Copy link

SparkQA commented Jan 14, 2015

Test build #25494 has started for PR 3897 at commit b5474ae.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Jan 14, 2015

Test build #25495 has started for PR 3897 at commit ddf7da4.

  • This patch merges cleanly.

@jongyoul
Copy link
Member Author

@mateiz I've fixed weird codes by your comments - thanks - and added a test case whether spark.executor.uri exists or not.

@jongyoul
Copy link
Member Author

@mateiz Could you please explain the purposes of coarse and fine-grained mode shortly? It looks similar. The only difference between two modes is the way to offer resources. I have no idea of history of existing two modes.

@SparkQA
Copy link

SparkQA commented Jan 14, 2015

Test build #25497 has started for PR 3897 at commit 6b8f169.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Jan 14, 2015

Test build #25497 has finished for PR 3897 at commit 6b8f169.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • command.setValue(s"cd $

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25497/
Test FAILed.

@SparkQA
Copy link

SparkQA commented Jan 14, 2015

Test build #25495 has finished for PR 3897 at commit ddf7da4.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • command.setValue(s"cd $

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25495/
Test PASSed.

@jongyoul
Copy link
Member Author

retest this please.I don't know why that simple test fails.

@SparkQA
Copy link

SparkQA commented Jan 14, 2015

Test build #25502 has started for PR 3897 at commit 6b8f169.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Jan 14, 2015

Test build #25494 has finished for PR 3897 at commit b5474ae.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • command.setValue(s"cd $

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25494/
Test PASSed.

@SparkQA
Copy link

SparkQA commented Jan 14, 2015

Test build #25502 has finished for PR 3897 at commit 6b8f169.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • command.setValue(s"cd $

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25502/
Test PASSed.

@jongyoul
Copy link
Member Author

@mateiz Passed tests. Please review it again.

@SparkQA
Copy link

SparkQA commented Jan 14, 2015

Test build #25533 has started for PR 3897 at commit e3cb711.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Jan 14, 2015

Test build #25533 has finished for PR 3897 at commit e3cb711.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • command.setValue(s"cd $

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25533/
Test PASSed.

@jongyoul
Copy link
Member Author

@mateiz Check this PR finally. I think It would be good by your review.

@mateiz
Copy link
Contributor

mateiz commented Jan 17, 2015

@jongyoul sorry for the delayed reply, but this looks good. Mind rebasing it on master though? It's now fallen behind.

@jongyoul
Copy link
Member Author

@mateiz I'll rebase this PR from master.

@SparkQA
Copy link

SparkQA commented Jan 19, 2015

Test build #25732 has started for PR 3897 at commit 25f3617.

  • This patch does not merge cleanly.

@jongyoul
Copy link
Member Author

Rebase is not finished.

- Changed command for using spark-class directly
- Delete sbin/spark-executor and moved some codes into spark-class' case statement
- Fixed code if spark.executor.uri doesn't have any value
- Added test cases
- Rebased from master

Conflicts:
	core/src/test/scala/org/apache/spark/scheduler/mesos/MesosSchedulerBackendSuite.scala
@SparkQA
Copy link

SparkQA commented Jan 19, 2015

Test build #25734 has started for PR 3897 at commit 932289f.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Jan 19, 2015

Test build #25732 has finished for PR 3897 at commit 25f3617.

  • This patch passes all tests.
  • This patch does not merge cleanly.
  • This patch adds the following public classes (experimental):
    • command.setValue(s"cd $

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25732/
Test PASSed.

@SparkQA
Copy link

SparkQA commented Jan 19, 2015

Test build #25734 has finished for PR 3897 at commit 932289f.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25734/
Test FAILed.

- Added a listenerBus for fixing test cases
@SparkQA
Copy link

SparkQA commented Jan 19, 2015

Test build #25738 has started for PR 3897 at commit 8232aa8.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Jan 19, 2015

Test build #25738 has finished for PR 3897 at commit 8232aa8.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25738/
Test PASSed.

@jongyoul
Copy link
Member Author

@mateiz I've rebased this PR and finished tests successfully. Merge this, please.

@pwendell
Copy link
Contributor

LGTM I will pull it in.

@asfgit asfgit closed this in 4a4f9cc Jan 19, 2015
@mateiz
Copy link
Contributor

mateiz commented Jan 20, 2015

Thanks, looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants