[Spark-15155][Mesos] Optionally ignore default role resources by hellertime · Pull Request #12933 · apache/spark

hellertime · 2016-05-05T13:34:47Z

Add new boolean property: spark.mesos.ignoreDefaultRoleResources. When this property is set Spark will only accept resources from the role passed in the spark.mesos.role property. If spark.mesos.role has not been set, spark.mesos.ignoreDefaultRoleResources has no effect.

Additional unit tests added to MesosSchedulerBackendSuite, extending the original multi-role test suite.

hellertime · 2016-05-05T13:35:30Z

@tnachen I'd like to request you review this change, since you are the author of the original mesos role code.

tnachen · 2016-05-05T14:21:37Z

So from what I can see, what you really want is to only don't select resources from * role right?
Otherwise if you're getting offers from other roles you are already registered for those roles as well, which begs the question why are you registered with that role as well.

tnachen · 2016-05-05T14:21:57Z

ok to test

SparkQA · 2016-05-05T14:23:42Z

Test build #57891 has finished for PR 12933 at commit 48b4723.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

hellertime · 2016-05-05T17:10:31Z

@tnachen exactly, I want to keep Spark from grabbing * roles, as in my use case I have a particular spark cluster that I want to isolate from other clusters.

SparkQA · 2016-05-05T18:16:44Z

Test build #57905 has finished for PR 12933 at commit 8419664.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

hellertime · 2016-05-05T18:47:25Z

MiMa test died here:

/home/jenkins/workspace/SparkPullRequestBuilder@2/dev/mima: line 37: 18083 Aborted (core dumped) java -XX:MaxPermSize=1g -Xmx2g -cp "$TOOLS_CLASSPATH:$OLD_DEPS_CLASSPATH" org.apache.spark.tools.GenerateMIMAIgnore
[error] running /home/jenkins/workspace/SparkPullRequestBuilder@2/dev/mima -Pyarn -Phadoop-2.3 -Pkinesis-asl -Phive-thriftserver -Phive ; received return code 134
Attempting to post to Github...
> Post successful.

I don't think this is due to my change, but I'm really not familiar with the MiMa test.

tnachen · 2016-05-06T03:04:08Z

I see, I wonder besides the wildcard role if it ever makes sense to have two lists of roles, one set that you are registered with and another set you want to accept resources for, I cannot think of any possible scenario.
Perhaps we should just introduce one boolean flag that default takes wildcard role resources or not? spark.mesos.rejectWildcardRole or something better named?

hellertime · 2016-05-06T03:20:01Z

I see that as a potential solution, however at some future point mesos will support frameworks which can hold multiple roles (MESOS-1763 😉) so perhaps leaving it in this form will ease that transition (though we'd then need to change the other property to spark.mesos.roles, so really its a wash).

tnachen · 2016-05-06T03:40:27Z

Yes I am aware of multi roles, but not sure in what situation you would want to register with multiple roles but only use one role's resources?

hellertime · 2016-05-06T03:44:55Z

True. So really the desired behavior is always just to ignore * resources. A boolean property would suffice here. What about spark.mesos.ignoreDefaultRoleResources, and have the property ignored if spark.mesos.role is unset?

hellertime · 2016-05-06T04:09:53Z

I'm updating the title of the PR to reflect the change in approach. I think the boolean property will be sufficient.

hellertime · 2016-05-06T12:08:19Z

Alright, I've modified the code to convert from a property which enumerated the accepted roles, to one which will simply ignore the default role when it is set.

I also removed one unit test, which was redundant with the test that checks we accepts all roles when spark.mesos.role is set.

SparkQA · 2016-05-06T13:32:35Z

Test build #57989 has finished for PR 12933 at commit 5ea2f46.

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

hellertime · 2016-05-06T13:51:19Z

Rebasing against master.

SparkQA · 2016-05-06T15:23:25Z

Test build #57993 has finished for PR 12933 at commit d2b7ad4.

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

tnachen · 2016-06-02T17:16:24Z

@hellertime Can you rebase and submit again to rerun the tests?

tnachen · 2016-06-22T03:15:37Z

@hellertime ping

hellertime · 2016-06-22T14:24:01Z

Hi! Was busy with the day job. I didn't mean to let this slip! Absolutely will rebase and retest. Thanks.

Sent from my iPhone

On Jun 21, 2016, at 11:16 PM, Timothy Chen notifications@github.com wrote:
@hellertime ping
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

tnachen · 2016-10-08T23:52:47Z

@hellertime Are you able to rebase?

hellertime · 2016-10-11T12:10:05Z

@tnachen rebased with master

SparkQA · 2016-10-11T12:13:55Z

Test build #66740 has finished for PR 12933 at commit e81ff73.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

hellertime · 2016-10-11T12:47:58Z

Fixed scala style error

SparkQA · 2016-10-11T12:58:34Z

Test build #66741 has finished for PR 12933 at commit 181f207.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-10-12T01:19:20Z

Test build #66780 has finished for PR 12933 at commit 838dc77.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-10-12T11:29:02Z

Test build #66811 has finished for PR 12933 at commit bbe908e.

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

SparkQA · 2016-10-12T15:23:38Z

Test build #66825 has finished for PR 12933 at commit 81e07c8.

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

hellertime · 2016-10-13T01:33:13Z

@tnachen I'm not sure what to do about this unit test failure. Running ./dev/run-tests on my system does not produce the error, and trying to just run the mesos suite with ./build/mvn -DwildcardSuites=org.apache.spark.scheduler.cluster.mesos test results in a successful build, it doesn't appear to actually run the test. Any advice on how to test this locally?

tnachen · 2016-10-13T04:23:08Z

I just tried running it locally and I'm getting the same error. It seems like with your change that test is simply declining the offer.

hellertime · 2016-10-13T10:20:43Z

You saw the error with ./dev/run-tests? Ok I'll figure this out.

Sent from my iPhone

On Oct 13, 2016, at 12:24 AM, Timothy Chen notifications@github.com wrote:
I just tried running it locally and I'm getting the same error. It seems like with your change that test is simply declining the offer.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

Add new boolean property: `spark.mesos.ignoreDefaultRoleResources`. When this property is set Spark will only accept resources from the role passed in the `spark.mesos.role` property. If `spark.mesos.role` has not been set, `spark.mesos.ignoreDefaultRoleResources` has no effect. Additional unit tests added to `MesosSchedulerBackendSuite`, extending the original multi-role test suite.

SparkQA · 2016-10-13T12:31:59Z

Test build #66887 has finished for PR 12933 at commit d64b261.

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

tnachen · 2016-10-13T16:08:32Z

I ran mvn test inside of the mesos folder.

On Thu, Oct 13, 2016 at 3:21 AM, Chris Heller notifications@github.com
wrote:

You saw the error with ./dev/run-tests? Ok I'll figure this out.
Sent from my iPhone
On Oct 13, 2016, at 12:24 AM, Timothy Chen notifications@github.com
wrote:
I just tried running it locally and I'm getting the same error. It seems
like with your change that test is simply declining the offer.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#12933 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAEGrAU4TKCiLBwMpDm7PmGsEm79tdoEks5qzgY0gaJpZM4IYBAd
.

tnachen · 2016-12-06T23:18:48Z

+ case true if role.isDefined => Set(role)
+ case _ => Set(Some("*"), role)
+ }
+ val acceptedRoles = roles.flatten


I think we should move this log outside of this helper method, as there might be other context in the future that's calling this.

tnachen · 2016-12-06T23:19:05Z

@srowen can you help review this? Besides my minor comment overall it looks fine with me.

srowen

I can only usefully comment on style and left a few comments

srowen · 2016-12-06T23:43:13Z

- getResource(o.resources, "cpus") >= driverCpu &&
- getResource(o.resources, "mem") >= driverMem
+ val acceptableResources = o.resources.asScala
+ .filter((r: Resource) => acceptedResourceRoles(r.getRole))


Just a style thing, but I think you'd generally just write (r => acceptedResourceRoles(r.getRole))

srowen · 2016-12-06T23:43:53Z

+ .filter((r: Resource) => acceptedResourceRoles(r.getRole))
+ .asJava
+ getResource(acceptableResources, "cpus") >= driverCpu &&
+ getResource(acceptableResources, "mem") >= driverMem


Indent this continuation 2 spaces to make it clear it's a continuation

srowen · 2016-12-06T23:46:32Z

+ * @param props Mesos driver description schedulerProperties map
+ */
+ protected def getAcceptedResourceRoles(props: Map[String, String]): Set[String] = {
+ getAcceptedResourceRoles(


This might be my taste only but I'm finding this style hard to read.
props.get(...).map(_.toBoolean).getOrElse(false)? It's what I'd expect to see in the spark code base but don't know if it's objectively any better. Still I'd break this out into a val rather than squeeze this into a method arg

srowen · 2016-12-06T23:47:30Z

+ */
+ private def getAcceptedResourceRoles(
+ ignoreDefaultRoleResources: Boolean,
+ role: Option[String]) = {


This seems more readable:
val roles = if (ignoreDefaultRoleResources && role.isDefined) Set(role) else Set(Some("*"), role)

HyukjinKwon · 2017-02-09T13:25:20Z

Hi @hellertime, this PR seems inactive for few months after the last review comments above. Would this be better closed if you are not currently able to work on this further maybe?

## What changes were proposed in this pull request? This PR proposes to close stale PRs. What I mean by "stale" here includes that there are some review comments by reviewers but the author looks inactive without any answer to them more than a month. I left some comments roughly a week ago to ping and the author looks still inactive in these PR below These below includes some PR suggested to be closed and a PR against another branch which seems obviously inappropriate. Given the comments in the last three PRs below, they are probably worth being taken over by anyone who is interested in it. Closesapache#7963Closesapache#8374Closesapache#11192Closesapache#11374Closesapache#11692Closesapache#12243Closesapache#12583Closesapache#12620Closesapache#12675Closesapache#12697Closesapache#12800Closesapache#13715Closesapache#14266Closesapache#15053Closesapache#15159Closesapache#15209Closesapache#15264Closesapache#15267Closesapache#15871Closesapache#15861Closesapache#16319Closesapache#16324Closesapache#16890Closesapache#12398Closesapache#12933Closesapache#14517 ## How was this patch tested? N/A Author: hyukjinkwon <gurwls223@gmail.com> Closesapache#16937 from HyukjinKwon/stale-prs-close.

hellertime changed the title ~~[Spark-15155][Mesos] Selectively accept Mesos resources by role~~[Spark-15155][Mesos] Optionally ignore default role resourcesMay 6, 2016

hellertime force-pushed the SPARK-15155 branch from 5ea2f46 to d2b7ad4Compare May 6, 2016 13:50

hellertime force-pushed the SPARK-15155 branch from d2b7ad4 to e81ff73Compare October 11, 2016 12:09

hellertime force-pushed the SPARK-15155 branch from e81ff73 to 181f207Compare October 11, 2016 12:47

hellertime force-pushed the SPARK-15155 branch from 181f207 to 838dc77Compare October 12, 2016 01:16

hellertime force-pushed the SPARK-15155 branch from 838dc77 to bbe908eCompare October 12, 2016 11:05

hellertime force-pushed the SPARK-15155 branch from bbe908e to 81e07c8Compare October 12, 2016 14:59

hellertime force-pushed the SPARK-15155 branch from 81e07c8 to d64b261Compare October 13, 2016 12:08

tnachen reviewed Dec 6, 2016
View reviewed changes

srowen requested changes Dec 6, 2016
View reviewed changes

HyukjinKwon mentioned this pull request Feb 15, 2017
[BUILD] Close stale PRs #16937
Closed

asfgit closed this in ed338f7Feb 17, 2017

Uh oh!

Conversation

hellertime commented May 5, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hellertime commented May 5, 2016

Uh oh!

tnachen commented May 5, 2016

Uh oh!

tnachen commented May 5, 2016

Uh oh!

SparkQA commented May 5, 2016

Uh oh!

hellertime commented May 5, 2016

Uh oh!

SparkQA commented May 5, 2016

Uh oh!

hellertime commented May 5, 2016

Uh oh!

tnachen commented May 6, 2016

Uh oh!

hellertime commented May 6, 2016

Uh oh!

tnachen commented May 6, 2016

Uh oh!

hellertime commented May 6, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hellertime commented May 6, 2016

Uh oh!

hellertime commented May 6, 2016

Uh oh!

SparkQA commented May 6, 2016

Uh oh!

hellertime commented May 6, 2016

Uh oh!

SparkQA commented May 6, 2016

Uh oh!

tnachen commented Jun 2, 2016

Uh oh!

tnachen commented Jun 22, 2016

Uh oh!

hellertime commented Jun 22, 2016

Uh oh!

tnachen commented Oct 8, 2016

Uh oh!

hellertime commented Oct 11, 2016

Uh oh!

SparkQA commented Oct 11, 2016

Uh oh!

hellertime commented Oct 11, 2016

Uh oh!

SparkQA commented Oct 11, 2016

Uh oh!

SparkQA commented Oct 12, 2016

Uh oh!

SparkQA commented Oct 12, 2016

Uh oh!

SparkQA commented Oct 12, 2016

Uh oh!

hellertime commented Oct 13, 2016

Uh oh!

tnachen commented Oct 13, 2016

Uh oh!

hellertime commented Oct 13, 2016

Uh oh!

SparkQA commented Oct 13, 2016

Uh oh!

tnachen commented Oct 13, 2016

Uh oh!

tnachenDec 6, 2016

Choose a reason for hiding this comment

Uh oh!

tnachen commented Dec 6, 2016

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

srowenDec 6, 2016

Choose a reason for hiding this comment

hellertime commented May 5, 2016•
edited
Loading

hellertime commented May 6, 2016•
edited
Loading