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.

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

[SPARK-12486] Worker should kill the executors more forcefully if possible. #10438

Closed
wants to merge 3 commits into from

Conversation

nongli
Copy link
Contributor

@nongli nongli commented Dec 22, 2015

This patch updates the ExecutorRunner's terminate path to use the new java 8 API
to terminate processes more forcefully if possible. If the executor is unhealthy,
it would previously ignore the destroy() call. Presumably, the new java API was
added to handle cases like this.

We could update the termination path in the future to use OS specific commands
for older java versions.

…sible.

This patch updates the ExecutorRunner's terminate path to use the new java 8 API
to terminate processes more forcefully if possible. If the executor is unhealthy,
it would previously ignore the destroy() call. Presumably, the new java API was
added to handle cases like this.
@SparkQA
Copy link

SparkQA commented Dec 22, 2015

Test build #48212 has finished for PR 10438 at commit 55032ea.

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

destroyMethod.setAccessible(true)
destroyMethod.invoke(process)
} catch {
case e: Exception => {
Copy link
Contributor

Choose a reason for hiding this comment

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

in general catch NonFatal, i.e.

case NonFatal(e) => process.destroy()

@andrewor14
Copy link
Contributor

Looks good.

} catch {
case e: Exception => {
process.destroy()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this might end up swallowing useful exception messages. Maybe we should catch more narrowly

case NonFatal(e) =>
  if (!e.isInstanceOf[NoSuchMethodException]) {
    logWarning("Exception when attempting to kill process", e)
  }
  process.destroy()

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for narrowly catching exceptions

@SparkQA
Copy link

SparkQA commented Dec 28, 2015

Test build #48374 has finished for PR 10438 at commit 67611ac.

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

if (!e.isInstanceOf[NoSuchMethodException]) {
logWarning("Exception when attempting to kill process", e)
} else {
process.destroy()
Copy link
Contributor

Choose a reason for hiding this comment

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

don't we want to best-effort destroy the process whether or not it's a NSME?

@yhuai
Copy link
Contributor

yhuai commented Dec 29, 2015

test this please

@SparkQA
Copy link

SparkQA commented Dec 29, 2015

Test build #48398 has finished for PR 10438 at commit 67611ac.

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

@SparkQA
Copy link

SparkQA commented Dec 31, 2015

Test build #48522 has finished for PR 10438 at commit 9ed4377.

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

@yhuai
Copy link
Contributor

yhuai commented Jan 4, 2016

test this please

@SparkQA
Copy link

SparkQA commented Jan 4, 2016

Test build #48624 has finished for PR 10438 at commit 9ed4377.

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

@andrewor14
Copy link
Contributor

Merging into master 1.6

asfgit pushed a commit that referenced this pull request Jan 4, 2016
…sible.

This patch updates the ExecutorRunner's terminate path to use the new java 8 API
to terminate processes more forcefully if possible. If the executor is unhealthy,
it would previously ignore the destroy() call. Presumably, the new java API was
added to handle cases like this.

We could update the termination path in the future to use OS specific commands
for older java versions.

Author: Nong Li <[email protected]>

Closes #10438 from nongli/spark-12486-executors.

(cherry picked from commit 8f65939)
Signed-off-by: Andrew Or <[email protected]>
@asfgit asfgit closed this in 8f65939 Jan 4, 2016
@nongli nongli deleted the spark-12486-executors branch January 4, 2016 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants