[SPARK-4525] MesosSchedulerBackend.resourceOffers cannot decline unused ... #3393

jongyoul · 2014-11-21T00:08:24Z

...offers from acceptedOffers

Added code for declining unused offers among acceptedOffers
Edited testCase for checking declining unused offers

…ed offers from acceptedOffers - Added code for declining unused offers among acceptedOffers - Edited testCase for checking declining unused offers

SparkQA · 2014-11-21T00:12:33Z

Test build #23696 has started for PR 3393 at commit f20f1b3.

This patch merges cleanly.

jongyoul · 2014-11-21T00:13:22Z

At launching from spark-submit with mesos master, for the first time, resourceOffers are called, there's no task at that time because taskScheduler didn't submit a job. Thus all offers are declined because no tasks. In branch-1.1, d.launchTasks with empty tasks declined implicitly, but in current master, If offers don't have any tasks, d.launchTasks are not called. Thus unused offers from acceptedOffers are not declined. I fix that situation and edited test case

tnachen · 2014-11-21T00:51:38Z

Good catch, I think I didn't completely understand how TaskSchedulerImpl are using the offers and forgot not all acceptable offers are eventually used. Your PR LGTM, +1

SparkQA · 2014-11-21T01:45:52Z

Test build #23696 has finished for PR 3393 at commit f20f1b3.

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

AmplabJenkins · 2014-11-21T01:45:56Z

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

jongyoul · 2014-11-21T01:48:39Z

@tnachen +1, Thanks.

jongyoul · 2014-11-21T17:10:41Z

@pwendell Please merge this PR. :-)

pwendell · 2014-11-22T21:46:09Z

I went ahead and reviewed this overall function (@andrewor14 merged some recent changes which @tnachen authored) and there seem to be multiple issues. Can you guys comment on the following?

The variable named acceptedOffers - why is it named this way if it doesn't indicate that the offers were accepted. Should it be acceptableOffers? Or usableOffers? When defining that variable it would be nice to have a comment that explains what is going on. Basically, you are excluding ahead of time nodes which we know ahead of time don't have enough memory to launch an executor or enough cores to launch even a single task. That should be in a comment.
Why does mesos need 2 * scheduler.CPUS_PER_TASK cores for a node to be considered and not 1 + scheduler.CPUS_PER_TASK? The executor needs a single core, it says so right in the comments.
Likewise when it subtracts a CPU for the executor, why does it subtract scheduler.CPUS_PER_TASK? Shouldn't it just subtract one?

As for this fix specifically. Right now it uses the absence of a node in slaveIdsWithExecutors to infer that the offer was not accepted. What happens if an offer is not used that is adding cores to an existing executor. Won't that offer be lost here and never replied to? It seems like to do this in a correct way we need to associate the returned task descriptions with resource offers.

I'm not an expert in this area of the code but a cursory glance at this functionality reveals some potential issues.

tnachen · 2014-11-22T22:17:06Z

Hi @pwendell, 1. Is a great suggestion there isn't enough comments for sure, I can add more in another pr.

For 2 I dont have enough context to infer what's the best choice of cores per task so I believe I just refactor the existing behavior. I do like to see why these values are chosen since this is more spark specific than mesos, I'll try to change these numbers and see if I can see any impact running spark perf.

tnachen · 2014-11-22T22:33:29Z

For 3, you are right if the offer is not used it is not acked.
My assumption was that all offers that we accepted based on the conditions including the ones expanding existing executors are used.
If that is not correct than we need to do what you suggested to have a local map to see what offers are used by the task scheduler impl.
I need to dig a bit deeper into the spark scheduler more.
@jongyoul i think if @pwendell condition can be true your fix needs to account that as well, sorry for the confusion

jongyoul · 2014-11-23T00:19:35Z

Basically, I thought minimum changes of code is a good way to fix bug, and refactoring is another issue.

@pwendell I agree that 1. As @tnachen mentioned above, he thought all acceptedOffers are eventually used. If I can refactor this code, It would be changed. usableOffers and unusableOffers are a better idea. then, unusableOffers would be declined explicitly.

3., MesosSchedulerBackend's CPUS_PER_TASK and slaveIdsWithExecutors are ambiguous. At first, I think 1 + scheduler.CPUS_PER_TASK is a better way than 2 * scheduler.CPUS_PER_TASK. Assumption of needing one more cpu is acceptable way because we should consider Mesos' worker is being launched in case that that executor is not added by slaveIdsWithExecutors.

I can fix and refactor that things and more by understanding that Impl deeply. Check my misunderstanding, please.

tnachen · 2014-11-23T00:51:56Z

Hi @jongyoul I think let's work on the cpu value in another PR.

Can you also add a test where the offer's slave id is already added but the offer wasn't used? Thanks!

pwendell · 2014-11-23T08:13:19Z

Thanks @tnachen and @jongyoul for answering questions. It seems like some of this behavior is just porting code that was in the old version of the Mesos binding before @tnachen refactored it. So we can hold off on clean-ups and do it separately.

We should fix the specific issue at hand though for 1.2. Looking back, I'm curious how this works in Spark 1.1. From what I can tell we never call declineOffer in Spark 1.1. However, we will call something like this:

d.launchTasks(Collections.singleton(offers(i).getId), mesosTasks(i), filters)

Where mesosTasks(i) is an empty list. @tnachen for mesos is that equivalent to declining the offer? The relevant code is here:

https://github.com/apache/spark/blob/branch-1.1/core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackend.scala#L263

tnachen · 2014-11-23T08:32:22Z

@pwendell Underneath the scheduler driver, declineOffer is actually just calling launchTasks with the offerId and an empty task vector as well, so the behavior is identical. I changed the api to call declineOffer since it's more semantically correct, and I believe more future-proof as the scheduler driver can change.

jongyoul · 2014-11-23T14:16:45Z

@pwendell Hi, @tnachen suggests that the issue of cpu divide with another issue. Is it better idea?

tnachen · 2014-11-23T21:46:20Z

@jongyoul I believe @pwendell is referring to the same issue that I was referring to, which is to handle the class that @pwendell has brought up.

pwendell · 2014-11-24T00:03:40Z

Yes - let's just fix the bug with this patch and we can punt improving this until later.

The bug with this patch is that if there is an offer that is not used for a node that is already in slavesWithExectors, then this patch will not decline it. One simple way to fix this is to keep track of nodes that were assigned an offer for that round, then decline offers that were not included in that set.

This assumes that we don't get multiple offers for a given host. However, I'm pretty sure the old code assumed that as well.

pwendell · 2014-11-24T00:04:05Z

@jongyoul if you could fix that bug and then add a unit test for that case, that would be great.

pwendell · 2014-11-24T00:06:33Z

Just to be totally clear, here is the case:

Spark already has an executor on node A. I.e. A is in slavesWithExecutors.
An additional core is offered on node A, but that offer is not used.
The offer is never declined.

tnachen · 2014-11-24T00:23:14Z

Yes you will only get one offer per slave in the same resourceoffers call. This might change once dynamic reservations are intoduced but shouldn't affect Spark unless spark uses this feature

pwendell · 2014-11-24T01:41:41Z

@tnachen do you agree that the issue I identified is a potential problem with the current proposed fix? Just want to make sure I'm not misunderstanding something.

tnachen · 2014-11-25T00:05:23Z

@pwendell yes it is a problem, we'll need to fix this before we can merge this.

tnachen · 2014-11-25T00:05:57Z

@pwendell Is this blocking 1.1.1? If @jongyoul can't get to it soon eonugh I can get a fix too.

pwendell · 2014-11-25T00:08:16Z

@tnachen. Not it's blocking me cutting a 1.2 RC though. I'm working on a small extension to this with new unit tests. I can post it in a few minutes.

tnachen · 2014-11-25T00:09:15Z

@pwendell sounds good, thx for identifying and fixing it! I'll take a look when you post it.

pwendell · 2014-11-25T00:16:05Z

Thanks @tnachen a review would be really appreciated

jongyoul · 2014-11-25T00:16:57Z

@tnachen I can fix this bug in a few days.

Functionally, this is just a small change on top of #3393 (by jongyoul). The issue being addressed is discussed in the comments there. I have not yet added a test for the bug there. I will add one shortly. I've also done some minor renaming/clean-up of variables in this class and tests. Author: Patrick Wendell <[email protected]> Author: Jongyoul Lee <[email protected]> Closes #3436 from pwendell/mesos-issue and squashes the following commits: 58c35b5 [Patrick Wendell] Adding unit test for this situation c4f0697 [Patrick Wendell] Additional clean-up and fixes on top of existing fix f20f1b3 [Jongyoul Lee] [SPARK-4525] MesosSchedulerBackend.resourceOffers cannot decline unused offers from acceptedOffers - Added code for declining unused offers among acceptedOffers - Edited testCase for checking declining unused offers

Functionally, this is just a small change on top of #3393 (by jongyoul). The issue being addressed is discussed in the comments there. I have not yet added a test for the bug there. I will add one shortly. I've also done some minor renaming/clean-up of variables in this class and tests. Author: Patrick Wendell <[email protected]> Author: Jongyoul Lee <[email protected]> Closes #3436 from pwendell/mesos-issue and squashes the following commits: 58c35b5 [Patrick Wendell] Adding unit test for this situation c4f0697 [Patrick Wendell] Additional clean-up and fixes on top of existing fix f20f1b3 [Jongyoul Lee] [SPARK-4525] MesosSchedulerBackend.resourceOffers cannot decline unused offers from acceptedOffers - Added code for declining unused offers among acceptedOffers - Edited testCase for checking declining unused offers (cherry picked from commit b043c27) Signed-off-by: Patrick Wendell <[email protected]>

pwendell · 2014-11-25T04:30:22Z

@jongyoul can you close this issue now? I pulled in your commits already.

…ed offers from acceptedOffers - Fix a case that unused node cannot be declined when slaveIdsWithExecutors has already that node.

jongyoul · 2014-11-25T05:14:36Z

@pwendell Oh, I'm late a little bit. I patched that code to similar you. I'll close this issue.

SparkQA · 2014-11-25T05:15:28Z

Test build #23814 has started for PR 3393 at commit 63855bf.

This patch does not merge cleanly.

jongyoul · 2014-11-25T05:20:17Z

@pwendell SparkQA trigger this issue for testing. Please check it and close this PR again. I did check your PR.

SparkQA · 2014-11-25T05:25:04Z

Test build #23815 has started for PR 3393 at commit 63855bf.

This patch does not merge cleanly.

pwendell · 2014-11-25T06:16:31Z

You should close this PR. I already merged my PR and gave you author credit:

f0afb62

SparkQA · 2014-11-25T07:05:00Z

Test build #23814 has finished for PR 3393 at commit 63855bf.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds no public classes.

AmplabJenkins · 2014-11-25T07:05:02Z

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

jongyoul · 2014-11-25T07:07:02Z

@pwendell Yes, I reopened beacuse jenkins triggered. I'll close again.

SparkQA · 2014-11-25T07:23:44Z

Test build #23815 has finished for PR 3393 at commit 63855bf.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds no public classes.

AmplabJenkins · 2014-11-25T07:23:48Z

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

[SPARK-4525] MesosSchedulerBackend.resourceOffers cannot decline unus…

f20f1b3

…ed offers from acceptedOffers - Added code for declining unused offers among acceptedOffers - Edited testCase for checking declining unused offers

pwendell mentioned this pull request Nov 25, 2014

[SPARK-4525] Mesos should decline unused offers #3436

Closed

[SPARK-4525] MesosSchedulerBackend.resourceOffers cannot decline unus…

63855bf

…ed offers from acceptedOffers - Fix a case that unused node cannot be declined when slaveIdsWithExecutors has already that node.

jongyoul closed this Nov 25, 2014

jongyoul reopened this Nov 25, 2014

jongyoul closed this Nov 25, 2014

[SPARK-4525] MesosSchedulerBackend.resourceOffers cannot decline unused ... #3393

[SPARK-4525] MesosSchedulerBackend.resourceOffers cannot decline unused ... #3393

Uh oh!

Conversation

jongyoul commented Nov 21, 2014

Uh oh!

SparkQA commented Nov 21, 2014

Uh oh!

jongyoul commented Nov 21, 2014

Uh oh!

tnachen commented Nov 21, 2014

Uh oh!

SparkQA commented Nov 21, 2014

Uh oh!

AmplabJenkins commented Nov 21, 2014

Uh oh!

jongyoul commented Nov 21, 2014

Uh oh!

jongyoul commented Nov 21, 2014

Uh oh!

pwendell commented Nov 22, 2014

Uh oh!

tnachen commented Nov 22, 2014

Uh oh!

tnachen commented Nov 22, 2014

Uh oh!

jongyoul commented Nov 23, 2014

Uh oh!

tnachen commented Nov 23, 2014

Uh oh!

pwendell commented Nov 23, 2014

Uh oh!

tnachen commented Nov 23, 2014

Uh oh!

jongyoul commented Nov 23, 2014

Uh oh!

tnachen commented Nov 23, 2014

Uh oh!

pwendell commented Nov 24, 2014

Uh oh!

pwendell commented Nov 24, 2014

Uh oh!

pwendell commented Nov 24, 2014

Uh oh!

tnachen commented Nov 24, 2014

Uh oh!

pwendell commented Nov 24, 2014

Uh oh!

tnachen commented Nov 25, 2014

Uh oh!

tnachen commented Nov 25, 2014

Uh oh!

pwendell commented Nov 25, 2014

Uh oh!

tnachen commented Nov 25, 2014

Uh oh!

pwendell commented Nov 25, 2014

Uh oh!

jongyoul commented Nov 25, 2014

Uh oh!

pwendell commented Nov 25, 2014

Uh oh!

jongyoul commented Nov 25, 2014

Uh oh!

SparkQA commented Nov 25, 2014

Uh oh!

jongyoul commented Nov 25, 2014

Uh oh!

SparkQA commented Nov 25, 2014

Uh oh!

pwendell commented Nov 25, 2014

Uh oh!

SparkQA commented Nov 25, 2014

Uh oh!

AmplabJenkins commented Nov 25, 2014

Uh oh!

jongyoul commented Nov 25, 2014

Uh oh!

SparkQA commented Nov 25, 2014

Uh oh!