Skip to content

[SPARK-21559][Mesos] remove mesos fine-grained mode #18784

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[SPARK-21559][Mesos] remove mesos fine-grained mode #18784

wants to merge 1 commit into from

Conversation

skonto
Copy link
Contributor

@skonto skonto commented Jul 31, 2017

What changes were proposed in this pull request?

Removes mesos fine-grained mode. Specifically:

  • Updates docs.
  • Renames spark.mesos.coarse* properties.
  • Removes logic for status updates in TaskSchedulerImpl (unique to fine-grained)
  • Removes MesosFineGrainedSchedulerBackend, MesosExecutorBackend and related tests.

How was this patch tested?

Tested with the integration test suite here:
https://github.com/typesafehub/mesos-spark-integration-tests

@skonto
Copy link
Contributor Author

skonto commented Jul 31, 2017

@ArtRand @susanxhuynh pls review.

@srowen
Copy link
Member

srowen commented Jul 31, 2017

Overall I trust your judgment if you think it's right to remove this, but I think it would have to wait for Spark 3.0?

@skonto
Copy link
Contributor Author

skonto commented Jul 31, 2017 •
edited
Loading

@srowen ok, yes. I have discussed this with Art and Suzan from Mesosphere and we made the decision to remove it as it is deprecated for so long. I am waiting for their comments here.
In the immediate future we will focus on coarse mode and we are planning to improve resource utilization & executor allocation per node, also coarse mode + dynamic allocation in general is a good solution.

@SparkQA
Copy link

SparkQA commented Jul 31, 2017

Test build #80072 has finished for PR 18784 at commit ce11906.

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

@skonto skonto force-pushed the remove_medos_fine_grained branch from ce11906 to 7dc41d0 Compare July 31, 2017 15:16
@skonto
Copy link
Contributor Author

skonto commented Jul 31, 2017

Fixed the test.

skonto
skonto commented Jul 31, 2017
View reviewed changes
core/src/test/scala/org/apache/spark/scheduler/TaskSchedulerImplSuite.scala Outdated
Copy link
Contributor Author

@skonto skonto Jul 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This test is only needed in the fine-grained case as it is the only case where the executor was killed in TaskSchedulerImpl. I am keeping the TaskStatus.LOST definition as it is propagated back from mesos processes to TaskSchedulerImpl.

@skonto skonto force-pushed the remove_medos_fine_grained branch from 7dc41d0 to 2c1574e Compare July 31, 2017 15:31
@skonto
Copy link
Contributor Author

skonto commented Jul 31, 2017

@srowen I made a new commit but didn't get a new build...

@SparkQA
Copy link

SparkQA commented Jul 31, 2017

Test build #80079 has finished for PR 18784 at commit 2c1574e.

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

@skonto
Copy link
Contributor Author

skonto commented Jul 31, 2017 •
edited
Loading

@srowen this test https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/80079/ seems flaky, I run the suite locally and works fine (13 tests finished successfully).

@skonto
Copy link
Contributor Author

skonto commented Jul 31, 2017

Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Jul 31, 2017

Test build #80087 has finished for PR 18784 at commit 2c1574e.

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

@skonto
Copy link
Contributor Author

skonto commented Aug 23, 2017

@ArtRand @susanxhuynh pls review.

@jiangxb1987
Copy link
Contributor

@skonto @ArtRand @susanxhuynh Do we still want this?

@ArtRand
Copy link

ArtRand commented Oct 16, 2017

@jiangxb1987 yes. I'll work to review this ASAP.

@imaxxs
Copy link

imaxxs commented Oct 18, 2017

I work with @ArtRand and @susanxhuynh Fine grained mode has been deprecated for a while. If it is standard procedure to wait till next release and if that is 3.0 we should wait till Spark 3.0 release. @skonto @srowen

@SparkQA
Copy link

SparkQA commented Mar 2, 2018

Test build #87876 has finished for PR 18784 at commit 2c1574e.

  • This patch fails due to an unknown error code, -9.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@MaxGekk MaxGekk mentioned this pull request Jun 26, 2018
Closed
@rxin
Copy link
Contributor

rxin commented Jul 18, 2018

Let's remove it in 3.0 then. We can do it after 2.4 release.

@skonto
Copy link
Contributor Author

skonto commented Aug 5, 2018

@rxin I will update the PR.

@skonto
Copy link
Contributor Author

skonto commented Nov 16, 2018

@imaxxs @rxin I think its a good time to remove this, I will update the PR if you are all ok.

@rxin
Copy link
Contributor

rxin commented Nov 16, 2018 via email •
edited
Loading

srowen
srowen reviewed Nov 21, 2018
View reviewed changes
Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

@skonto also cool by me. If you resolve the merge conflict, let's do it.

docs/running-on-mesos.md
"fine-grained" (deprecated).

## Coarse-Grained
Spark runs over Mesos in "coarse-grained" mode.
Copy link
Member

Choose a reason for hiding this comment

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

I might add a note here that fine-grained was removed in 3.0.0

...main/scala/org/apache/spark/scheduler/cluster/mesos/MesosCoarseGrainedSchedulerBackend.scala
private[this] val shutdownTimeoutMS =
conf.getTimeAsMs("spark.mesos.coarse.shutdownTimeout", "10s")
.ensuring(_ >= 0, "spark.mesos.coarse.shutdownTimeout must be >= 0")
conf.getTimeAsMs("spark.mesos.shutdownTimeout", "10s")
Copy link
Member

Choose a reason for hiding this comment

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

Does this property name change need to be reflected in docs? I wasn't sure.

@srowen
Copy link
Member

srowen commented Dec 3, 2018

@skonto do you want to proceed with this?

@HyukjinKwon
Copy link
Member

Looks inactive. @srowen and @felixcheung, do you know anyone who might be interested in this?

@skonto
Copy link
Contributor Author

skonto commented Feb 4, 2019 •
edited
Loading

@srowen @HyukjinKwon sorry missed the msg, yes I will spend some cycles to fix it.

@vanzin
Copy link
Contributor

vanzin commented Mar 4, 2019

Ping? If you won't update this, please close so someone else can pick it up.

@felixcheung
Copy link
Member

let's do it!
it's only 2017 though. maybe we should wait until July 2019 its 2nd year anniversary to merge this? (ok, I'm joking)

@skonto
Copy link
Contributor Author

skonto commented May 16, 2019

Ok let me see what I can update before closing.

@skonto
Copy link
Contributor Author

skonto commented May 22, 2019

closing.

@skonto skonto closed this May 22, 2019
@felixcheung
Copy link
Member

felixcheung commented May 25, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srowen srowen srowen left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@skonto @srowen @SparkQA @jiangxb1987 @ArtRand @imaxxs @rxin @HyukjinKwon @vanzin @felixcheung