search

lightbend / mesos-spark-integration-tests

Mesos Integration Tests on Docker/Ec2
16 stars 9 forks source link

Added DCOS Spec #57

Closed mgummelt closed 8 years ago

mgummelt commented 8 years ago

cc @skonto

skonto commented 8 years ago

LGTM, only minor issues. I have run it locally runs fine.

Note: However, I am not so sure about the role tests. My opinion is that in local mode we can implicitly deduce if role resources are utilized although surprisingly mesos lacks of an api to ask for which type of resources are offered to and used by a framework any given time. Moreover, this info does exist in some form in the master log where you can see what resources were allocated.

Anyway, if everyone is ok we can fall to back to the simple assert if a role set exists in spark. Maybe in the future we could revise this.

dragos commented 8 years ago

@mgummelt A bit of context here: https://issues.apache.org/jira/browse/SPARK-10749. This was one of the motivators for this effort /cc @tnachen

The idea was to have an integration test for this scenario. If it is impossible, we can skip it and assume that the unit test will catch it.

It is true that we're assuming a controlled Mesos cluster, and running it in a multi-tenant setting is something we didn't consider. Happy to skip such tests in the DCOS suite, I'm just wondering if there's value in keeping it for the single-user case (docker).

mgummelt commented 8 years ago

This won't actually test SPARK-10749 directly, since this is testing the driver, and SPARK-10749 was a bug with the dispatcher, but I see your point. We want to test that a driver will correctly use an offer for a given role.

I have a few thoughts about this. I'll send an email.

mgummelt commented 8 years ago

@skonto ready for a review

skonto commented 8 years ago

ok im reviewing it... sorry for the delay...

skonto commented 8 years ago

61 due to this i cannot completely test it... but im still reviewing the code

skonto commented 8 years ago

now im back on it....

skonto commented 8 years ago

@mgummelt looks fine locally... could you check my comments above? @skyluc @dragos any comments?

mgummelt commented 8 years ago

@skonto I'm using call-by-value now. All good?

mgummelt commented 8 years ago

b.t.w I forked the master branch into our own custom version so we have more control over when tests get fixed. We're not planning on diverging, we just need the ability to merge hotfixes in ASAP.

skonto commented 8 years ago

I understand that but proper reviews need time...im not sure if that will work for you while somehow not diverging because the order that prs are merged will be different, prs maybe rejected or changed... am i wrong?

skonto commented 8 years ago

LGTM.

skyluc commented 8 years ago

One nitpicking commend, otherwise LGTM.

mgummelt commented 8 years ago

@skonto we'll still be merging in all upstream changes. If there's a PR we need to fix tests, but it doesn't get accepted upstream, then we'll have to figure out what to do with it on a case-by-case basis. If it's absolutely needed, they there will be a slight divergence, yes. But that should be the exception. We're still going to be submitting all our PRs against master and contributing upstream.