[SPARK-21383][Core] Fix the YarnAllocator allocates more Resource #18651

libratiger · 2017-07-17T08:00:43Z

What changes were proposed in this pull request?

When NodeManagers launching Executors,
the missing value will exceed the
real value when the launch is slow, this can lead to YARN allocates more resource.

We add the numExecutorsRunning when calculate the missing to avoid this.

How was this patch tested?

Test by experiment.

When NodeManagers launched the Executors, the missing will excel the real value, this can lead to YARN allocate more resource.

vanzin · 2017-07-18T18:10:03Z

ok to test

vanzin · 2017-07-18T18:10:24Z

the missing value will excel the

Not sure what that means. Maybe "exceed"?

SparkQA · 2017-07-18T18:29:07Z

Test build #79716 has finished for PR 18651 at commit 818c912.

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

vanzin · 2017-07-18T21:07:52Z

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnAllocator.scala

-                  // occupation.
-                  amClient.releaseAssignedContainer(containerId)
+          try {
+            numExecutorToBeLaunched += 1


This is not thread safe. += is two operations.

vanzin · 2017-07-18T21:08:03Z

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnAllocator.scala

-          })
+            })
+          } finally {
+            numExecutorToBeLaunched -= 1


Also, shouldn't this be done in the actual thread that launching the executor? Otherwise you're decrementing the counter as soon as the task is submitted for execution, but you're not really waiting for the task to execute.

Yes, you're right. When I test the code by experiment, i decrease the numExecutorToBeLaunched in the updateInternalState function, but I later found this may impact the test.

I will fix this soon.

vanzin · 2017-07-18T21:08:59Z

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnAllocator.scala


  @volatile private var numExecutorsRunning = 0

+  @volatile private var numExecutorToBeLaunched = 0


A better name would be numExecutorsStarting.

OK，I will change the name.

When NodeManagers launched the Executors, the missing will excel the real value, this can lead to YARN allocate more resource.

libratiger · 2017-07-19T07:45:10Z

I just update the code, and test by experiment, can you take a look at @vanzin

SparkQA · 2017-07-19T08:00:15Z

Test build #79743 has finished for PR 18651 at commit 856ebd6.

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

SparkQA · 2017-07-19T08:04:03Z

Test build #79745 has finished for PR 18651 at commit bf7f78c.

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

tgravescs · 2017-07-19T13:36:46Z

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnAllocator.scala

        .format(
          allocatedContainers.size,
-          numExecutorsRunning,
+          numExecutorsRunning.get,


it woudl be nice to print the numExecutorsStarting here as well.

tgravescs · 2017-07-19T13:42:28Z

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnAllocator.scala

-    val missing = targetNumExecutors - numPendingAllocate - numExecutorsRunning
+    val missing = targetNumExecutors - numPendingAllocate -
+      numExecutorsStarting.get - numExecutorsRunning.get



can you also add in a debug message here, something like below. I found this very useful when debugging this issue and think it would be useful for debugging other allocating issues in the future.

logDebug(s"Updating resource requests, target: $targetNumExecutors, pending: " +

s"$numPendingAllocate, running: $numExecutorsRunning, executorsStarting $numExecutorsPendingStart")

Thanks for your advice! I just add the debug info.

SparkQA · 2017-07-19T16:16:26Z

Test build #79764 has finished for PR 18651 at commit cc73e5b.

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

vanzin · 2017-07-19T19:10:31Z

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnAllocator.scala

    if (executorIdToContainer.contains(executorId)) {
      val container = executorIdToContainer.get(executorId).get
      internalReleaseContainer(container)
-      numExecutorsRunning -= 1


This doesn't need to be an atomic integer because this method is synchronized already - and so is the other method where it's modified.

Yes, I just try to keep consistency with numExecutorsStarting

vanzin

The changes to numExecutorsRunning are not necessary, but at the same time they don't hurt. I'll let Tom take a look since he left feedback earlier.

vanzin · 2017-07-19T19:12:13Z

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnAllocator.scala

-                  // occupation.
+                  // Assigned container should be released immediately
+                  // to avoid unnecessary resource occupation.
+                  numExecutorsStarting.decrementAndGet()


Safer to put this in a finally, no?

Yes, it is more robust. I have update the code.

SparkQA · 2017-07-20T06:49:14Z

Test build #79789 has finished for PR 18651 at commit 582f424.

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

tgravescs · 2017-07-21T16:44:05Z

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnAllocator.scala

+                  // to avoid unnecessary resource occupation.
                  amClient.releaseAssignedContainer(containerId)
+              } finally {
+                numExecutorsStarting.decrementAndGet()


why did you move this out of updateInternalState? I would rather see those stay together because updateInternalState is synchronized and if they aren't changed at the same time you could end up with the wrong numbers. numExecutorsStarting could still be say 1 when you have already added that to the # running and then updateResourceRequests gets an incorrect number.

If you move this to updateInternalState, then if there's a problem in ExecutorRunnable.run this counter will not be decremented.

updateInternalState is called right after the run if it succeeds. You do still need to handle the failure as well but in the failure the numExecutorsRunning is never incremented. I would rather see those handled separately and make sure we don't over count.

this is "starting" not "running".

Note I thought the last version handled this properly where he had the decrement in the catch block

yes but its a bug right now as the numbers can be wrong. Are you looking at the synchronization?

Right now everything is called synchronized up to the point of launcher pool to do the ExecutorRunnable. At this point running is not incremented, pending is decremented and we now increment Starting. That is fine.

But when the ExecutorRunnable finishes the only place its called synchronized is in updateInternalState. This right now increments running but does not decrement starting. if updateResourceRequests gets called (which is synchronized), Right after updateInternalState (which leave the syncrhonized) but before the finally block executes and decrements starting the total number can be more then it really is. That executor is counted as both running and starting

Ok, I see. The current code can double count the same executor as starting and running, while the previous code could count it as starting even though it failed to start (for a really small window), but that is a self-healing situation while the previous can have some adverse effects.

yep, the fact that its still marked as Starting even though failed will fix itself next loop through. Its no different then if we didn't know it failed and it was still in the ExecutorRunnable.run code.

I agree that put numExecutorsStarting.decrementAndGet() together with numExecutorsRunning.incrementAndGet() in the updateInternalState is better if we can.

Why I try to put numExecutorsStarting.decrementAndGet() in the finally block is that if there some Exceptions is not NonFatal, and caught by the following code, we may can not allocated resources as we specified, this is the same as @vanzin worried.

We may double the count in the current code, but this only slow down the allocation rate for a small time.

If its a fatal error I don't really expect us to continue so dynamic allocation doesn't matter. If you know a case where we would recover from fatal exception, I'm fine with adding in another catch there to decrement in the fatal case as well.

tgravescs · 2017-07-21T18:21:26Z

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnAllocator.scala

-      if (numExecutorsRunning < targetNumExecutors) {
+      if (numExecutorsRunning.get < targetNumExecutors) {
        if (launchContainers) {
+          numExecutorsStarting.incrementAndGet()


note if we change the below back this needs to move outside if(launchContainers)

libratiger · 2017-07-25T09:53:33Z

I update the code, please take a look at @vanzin @tgravescs

SparkQA · 2017-07-25T10:03:59Z

Test build #79928 has finished for PR 18651 at commit 7703494.

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

tgravescs · 2017-07-25T14:33:56Z

LGTM. @vanzin anything further?

vanzin · 2017-07-25T17:19:36Z

Merging to master / 2.2.

When NodeManagers launching Executors, the `missing` value will exceed the real value when the launch is slow, this can lead to YARN allocates more resource. We add the `numExecutorsRunning` when calculate the `missing` to avoid this. Test by experiment. Author: DjvuLee <[email protected]> Closes #18651 from djvulee/YarnAllocate. (cherry picked from commit 8de080d) Signed-off-by: Marcelo Vanzin <[email protected]>

When NodeManagers launching Executors, the `missing` value will exceed the real value when the launch is slow, this can lead to YARN allocates more resource. We add the `numExecutorsRunning` when calculate the `missing` to avoid this. Test by experiment. Author: DjvuLee <[email protected]> Closes apache#18651 from djvulee/YarnAllocate.

When NodeManagers launching Executors, the `missing` value will exceed the real value when the launch is slow, this can lead to YARN allocates more resource. We add the `numExecutorsRunning` when calculate the `missing` to avoid this. Test by experiment. Author: DjvuLee <[email protected]> Closes apache#18651 from djvulee/YarnAllocate. (cherry picked from commit 8de080d) Signed-off-by: Marcelo Vanzin <[email protected]>

[Core] Fix the YarnAllocator allocate more Resource

818c912

When NodeManagers launched the Executors, the missing will excel the real value, this can lead to YARN allocate more resource.

vanzin reviewed Jul 18, 2017

View reviewed changes

DjvuLee added 4 commits July 19, 2017 11:23

[Core] Fix the YarnAllocator allocate more Resource

aba617e

When NodeManagers launched the Executors, the missing will excel the real value, this can lead to YARN allocate more resource.

Fix the concurrent problem

0980a6a

Fix the merge conflict

856ebd6

Remove unused code

bf7f78c

tgravescs reviewed Jul 19, 2017

View reviewed changes

Add the debug log

cc73e5b

vanzin reviewed Jul 19, 2017

View reviewed changes

make more robust

582f424

tgravescs reviewed Jul 21, 2017

View reviewed changes

Fix the double count bug

7703494

asfgit closed this in 8de080d Jul 25, 2017

jcuquemelle mentioned this pull request Nov 24, 2017

[SPARK-21383][YARN] Fix the YarnAllocator allocates more Resource criteo-forks/spark#61

Merged


		@volatile private var numExecutorsRunning = 0

		@volatile private var numExecutorToBeLaunched = 0

[SPARK-21383][Core] Fix the YarnAllocator allocates more Resource #18651

[SPARK-21383][Core] Fix the YarnAllocator allocates more Resource #18651

Uh oh!

Conversation

libratiger commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

vanzin commented Jul 18, 2017

Uh oh!

vanzin commented Jul 18, 2017

Uh oh!

SparkQA commented Jul 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

libratiger commented Jul 19, 2017

Uh oh!

SparkQA commented Jul 19, 2017

Uh oh!

SparkQA commented Jul 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 19, 2017

Uh oh!

vanzin Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vanzin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgravescs Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

libratiger commented Jul 17, 2017 •

edited

Loading

vanzin Jul 19, 2017 •

edited

Loading

tgravescs Jul 21, 2017 •

edited

Loading