Skip to content

Conversation

@mgummelt
Copy link

@mgummelt mgummelt commented Feb 23, 2017

What changes were proposed in this pull request?

See JIRA

How was this patch tested?

Unit tests, Mesos/Spark integration tests

cc @skonto @susanxhuynh

…ather than acquired cores in the Mesos Coarse Grained Scheduler
@mgummelt mgummelt changed the title [SPARK-19373] Base spark.scheduler.minRegisteredResourceRatio on registered cores r… [SPARK-19373][MESOS] fix spark.scheduler.minRegisteredResourceRatio Feb 23, 2017

override def sufficientResourcesRegistered(): Boolean = {
totalCoresAcquired >= maxCores * minRegisteredRatio
totalCoreCount.get >= maxCoresOption.getOrElse(0) * minRegisteredRatio
Copy link
Author

Choose a reason for hiding this comment

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

This is the only substantive change.

Copy link
Contributor

@skonto skonto Feb 28, 2017

Choose a reason for hiding this comment

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

Just to clarify stuff. totalCoreCount holds the total number of cores in the cluster registered by executors connected back to the scheduler. So as soon as you have the all executors connected you can start the tasks, instead of gradually registering executors and running tasks on them.

Copy link
Author

Choose a reason for hiding this comment

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

Yep.

@SparkQA
Copy link

SparkQA commented Feb 24, 2017

Test build #73370 has finished for PR 17045 at commit ffbc76c.

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

@SparkQA
Copy link

SparkQA commented Feb 24, 2017

Test build #73371 has finished for PR 17045 at commit a234985.

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

@markhamstra
Copy link
Contributor

Please avoid using "fix" as the description in a PR -- it doesn't tell us anything substantive about the nature of the problem or its resolution, so any future reviewing of commit messages will require digging into the actual committed code to get even a minimal idea of what was done.

@mgummelt mgummelt changed the title [SPARK-19373][MESOS] fix spark.scheduler.minRegisteredResourceRatio [SPARK-19373][MESOS] Base spark.scheduler.minRegisteredResourceRatio on registered cores rather than accepted cores Feb 24, 2017
@mgummelt
Copy link
Author

@markhamstra Updated

@markhamstra
Copy link
Contributor

thanks

@SparkQA
Copy link

SparkQA commented Feb 24, 2017

Test build #73441 has finished for PR 17045 at commit 6818ae0.

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

@skonto
Copy link
Contributor

skonto commented Feb 28, 2017

LGTM run it locally works fine.

@mgummelt
Copy link
Author

mgummelt commented Feb 28, 2017

@srowen Can we get a merge? This is a bugfix, so it potentially belongs in all supported branches (1.6, 2.0, 2.1, master). It's not a major bug, though, so I'll leave the backport decisions up to you.

@asfgit asfgit closed this in ca3864d Feb 28, 2017
@srowen
Copy link
Member

srowen commented Feb 28, 2017

Merged to master. The cherry-pick wasn't clean into 2.1, and it wasn't entirely trivial. Rather than try to fix and get it wrong, if you'd care to open a PR against 2.1 I can merge that back and see how far that gets.

mgummelt pushed a commit to d2iq-archive/spark that referenced this pull request Mar 1, 2017
…on registered cores rather than accepted cores

See JIRA

Unit tests, Mesos/Spark integration tests

cc skonto susanxhuynh

Author: Michael Gummelt <mgummelt@mesosphere.io>

Closes apache#17045 from mgummelt/SPARK-19373-registered-resources.
asfgit pushed a commit that referenced this pull request Mar 1, 2017
…on registered cores rather than accepted cores

See JIRA

Unit tests, Mesos/Spark integration tests

cc skonto susanxhuynh

Author: Michael Gummelt <mgummeltmesosphere.io>

Closes #17045 from mgummelt/SPARK-19373-registered-resources.

## What changes were proposed in this pull request?

(Please fill in changes proposed in this fix)

## How was this patch tested?

(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)
(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)

Please review http://spark.apache.org/contributing.html before opening a pull request.

Author: Michael Gummelt <mgummelt@mesosphere.io>

Closes #17129 from mgummelt/SPARK-19373-registered-resources-2.1.
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.

5 participants