Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SPARK-23637][YARN]Yarn might allocate more resource if a same executor is killed multiple times. #20781

Closed
wants to merge 3 commits into from
Closed

[SPARK-23637][YARN]Yarn might allocate more resource if a same executor is killed multiple times. #20781

wants to merge 3 commits into from

Conversation

jinxing64
Copy link

What changes were proposed in this pull request?

YarnAllocator uses numExecutorsRunning to track the number of running executor. numExecutorsRunning is used to check if there're executors missing and need to allocate more.

In current code, numExecutorsRunning can be negative when driver asks to kill a same idle executor multiple times.

How was this patch tested?

UT added

@SparkQA
Copy link

SparkQA commented Mar 9, 2018

Test build #88116 has finished for PR 20781 at commit bd6f8a1.

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

@jinxing64
Copy link
Author

cc @vanzin @tgravescs @cloud-fan @djvulee
Could you please help review this ?

@jerryshao
Copy link
Contributor

jerryshao commented Mar 9, 2018 •
edited
Loading

Does it happen only in dynamic allocation enabled scenario?

NVM.

can be negative when driver asks to kill a same idle executor multiple times.

Can you please describe how this happened?

@jinxing64
Copy link
Author

jinxing64 commented Mar 9, 2018 •
edited
Loading

@jerryshao Thanks for taking look.

Yes, it does happen. we have jobs which have already finished all the tasks but still holding 40~100 executors.

Well I'm not sure if it exists in non dynamic scenario.

@jerryshao
Copy link
Contributor

This basically means that drive send multiple same kill requests to AM, right? I'm wondering how this would happen, shall we also guarantee this in the driver side?

@jinxing64
Copy link
Author

jinxing64 commented Mar 9, 2018 •
edited
Loading

@jerryshao
Thanks for advice. I spent some time digging to find why multiple kill sent from Driver to AM, but didn't figure out a way to reproduce.

I come to find that it's possible YarnAllocator process same completed container multiple times(https://github.com/apache/spark/blob/master/resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnAllocator.scala#L573 this log is printed multiple times for same container), which can also make numExecutorsRunning negative. And I made another change(see last commit in this pr) to propose my idea -- replace numExecutorsRunning with a set.

@jinxing64
Copy link
Author

Since the change for YarnAllocator: killExecutor is easy. Do you think it's worth to have this defense?
Thanks again for review.

@SparkQA
Copy link

SparkQA commented Mar 9, 2018

Test build #88127 has finished for PR 20781 at commit a177a63.

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

jerryshao
jerryshao reviewed Mar 12, 2018
View reviewed changes
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnAllocator.scala Outdated
@@ -81,7 +81,7 @@ private[yarn] class YarnAllocator(
private val releasedContainers = Collections.newSetFromMap[ContainerId](
new ConcurrentHashMap[ContainerId, java.lang.Boolean])

private val numExecutorsRunning = new AtomicInteger(0)
private val runningExecutors = new java.util.concurrent.ConcurrentHashMap[String, Unit]()
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be changed to Collections.newSetFromMap, since we only need Set instead of Map.

@jerryshao
Copy link
Contributor

Still I'm not so sure about the root cause, but adding defensive code seems no harm.

@jinxing64
Copy link
Author

jinxing64 commented Mar 12, 2018 •
edited
Loading

@jerryshao
Thanks again for review.
It does exist in my cluster that same completed container can be processed multiple times, which will make numExecutorsRunning negative. I think I've ever seen such issue in another Spark jira, but I cannot find it now.

@SparkQA
Copy link

SparkQA commented Mar 12, 2018

Test build #88178 has finished for PR 20781 at commit 049ed49.

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

@vanzin
Copy link
Contributor

vanzin commented Mar 12, 2018

The change looks good, but did you look at why the code is trying to kill the same executor multiple times? That sounds like it could be a possible bug on the scheduler backend, which should be keeping track of these things.

@jinxing64
Copy link
Author

@vanzin
Thanks for review~

  1. I spent some time but didn't find the reason why same executor is killed multiple times and I cannot reproduce either.
  2. I found that same completed container can be processed multiple times. It happens now and then. Seems yarn doesn't promise that same completed container only returned in one response (https://github.com/apache/spark/blob/master/resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnAllocator.scala#L268)

@vanzin
Copy link
Contributor

vanzin commented Apr 2, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Apr 2, 2018

Test build #88837 has finished for PR 20781 at commit 049ed49.

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

@vanzin
Copy link
Contributor

vanzin commented Apr 4, 2018

Merging to master / 2.3.

asfgit pushed a commit that referenced this pull request Apr 4, 2018
[SPARK-23637][YARN] Yarn might allocate more resource if a same execu…
0b7b8cc 
…tor is killed multiple times.

## What changes were proposed in this pull request?
`YarnAllocator` uses `numExecutorsRunning` to track the number of running executor. `numExecutorsRunning` is used to check if there're executors missing and need to allocate more.

 In current code, `numExecutorsRunning` can be negative when driver asks to kill a same idle executor multiple times.

## How was this patch tested?
UT added

Author: jinxing <[email protected]>

Closes #20781 from jinxing64/SPARK-23637.

(cherry picked from commit d3bd043)
Signed-off-by: Marcelo Vanzin <[email protected]>
@asfgit asfgit closed this in d3bd043 Apr 4, 2018
mshtelma pushed a commit to mshtelma/spark that referenced this pull request Apr 5, 2018
[SPARK-23637][YARN] Yarn might allocate more resource if a same execu…
984faf5 
…tor is killed multiple times.

## What changes were proposed in this pull request?
`YarnAllocator` uses `numExecutorsRunning` to track the number of running executor. `numExecutorsRunning` is used to check if there're executors missing and need to allocate more.

 In current code, `numExecutorsRunning` can be negative when driver asks to kill a same idle executor multiple times.

## How was this patch tested?
UT added

Author: jinxing <[email protected]>

Closes apache#20781 from jinxing64/SPARK-23637.
robert3005 pushed a commit to palantir/spark that referenced this pull request Apr 7, 2018
[SPARK-23637][YARN] Yarn might allocate more resource if a same execu…
ff98d78 
…tor is killed multiple times.

## What changes were proposed in this pull request?
`YarnAllocator` uses `numExecutorsRunning` to track the number of running executor. `numExecutorsRunning` is used to check if there're executors missing and need to allocate more.

 In current code, `numExecutorsRunning` can be negative when driver asks to kill a same idle executor multiple times.

## How was this patch tested?
UT added

Author: jinxing <[email protected]>

Closes apache#20781 from jinxing64/SPARK-23637.
@jinxing64
Copy link
Author

@vanzin Thanks for merging.

peter-toth pushed a commit to peter-toth/spark that referenced this pull request Oct 6, 2018
[SPARK-23637][YARN] Yarn might allocate more resource if a same execu…
47f166c 
…tor is killed multiple times.

`YarnAllocator` uses `numExecutorsRunning` to track the number of running executor. `numExecutorsRunning` is used to check if there're executors missing and need to allocate more.

 In current code, `numExecutorsRunning` can be negative when driver asks to kill a same idle executor multiple times.

UT added

Author: jinxing <[email protected]>

Closes apache#20781 from jinxing64/SPARK-23637.

(cherry picked from commit d3bd043)
Signed-off-by: Marcelo Vanzin <[email protected]>

Change-Id: I5b70fa55343828b303f96e0672525259f33e43ab
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jerryshao jerryshao jerryshao left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jinxing64 @SparkQA @jerryshao @vanzin