[SPARK-16182] [CORE] Utils.scala -- terminateProcess() should call Process.destroyForcibly() if and only if Process.destroy() fails #13973
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…estroyForcibly() if it fails. Utils.waitForProcess should use Java 8 method if available.
|
Test build #61469 has finished for PR 13973 at commit
|
asfgit
pushed a commit
that referenced
this pull request
Jul 1, 2016
…cess.destroyForcibly() if and only if Process.destroy() fails ## What changes were proposed in this pull request? Utils.terminateProcess should `destroy()` first and only fall back to `destroyForcibly()` if it fails. It's kind of bad that we're force-killing executors -- and only in Java 8. See JIRA for an example of the impact: no shutdown While here: `Utils.waitForProcess` should use the Java 8 method if available instead of a custom implementation. ## How was this patch tested? Existing tests, which cover the force-kill case, and Amplab tests, which will cover both Java 7 and Java 8 eventually. However I tested locally on Java 8 and the PR builder will try Java 7 here. Author: Sean Owen <[email protected]> Closes #13973 from srowen/SPARK-16182. (cherry picked from commit 2075bf8) Signed-off-by: Sean Owen <[email protected]>
Member
Author
|
Merged to master/2.0/1.6. I went ahead with this as I think it's a reasonably important fix for something that only manifests in Java 8, that we hard-kill executors and don't let them for example finish shutdown hook cleanup at all. |
asfgit
pushed a commit
that referenced
this pull request
Jul 1, 2016
…cess.destroyForcibly() if and only if Process.destroy() fails ## What changes were proposed in this pull request? Utils.terminateProcess should `destroy()` first and only fall back to `destroyForcibly()` if it fails. It's kind of bad that we're force-killing executors -- and only in Java 8. See JIRA for an example of the impact: no shutdown While here: `Utils.waitForProcess` should use the Java 8 method if available instead of a custom implementation. ## How was this patch tested? Existing tests, which cover the force-kill case, and Amplab tests, which will cover both Java 7 and Java 8 eventually. However I tested locally on Java 8 and the PR builder will try Java 7 here. Author: Sean Owen <[email protected]> Closes #13973 from srowen/SPARK-16182. (cherry picked from commit 2075bf8) Signed-off-by: Sean Owen <[email protected]>
zzcclp
pushed a commit
to zzcclp/spark
that referenced
this pull request
Jul 1, 2016
…cess.destroyForcibly() if and only if Process.destroy() fails ## What changes were proposed in this pull request? Utils.terminateProcess should `destroy()` first and only fall back to `destroyForcibly()` if it fails. It's kind of bad that we're force-killing executors -- and only in Java 8. See JIRA for an example of the impact: no shutdown While here: `Utils.waitForProcess` should use the Java 8 method if available instead of a custom implementation. ## How was this patch tested? Existing tests, which cover the force-kill case, and Amplab tests, which will cover both Java 7 and Java 8 eventually. However I tested locally on Java 8 and the PR builder will try Java 7 here. Author: Sean Owen <[email protected]> Closes apache#13973 from srowen/SPARK-16182. (cherry picked from commit 2075bf8) Signed-off-by: Sean Owen <[email protected]> (cherry picked from commit 83f8604)
Contributor
|
@srowen Seems this commit breaks 1.6 builds (https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/job/spark-branch-1.6-test-sbt-hadoop-1.0/248/)? |
Member
Author
dongjoon-hyun
pushed a commit
that referenced
this pull request
Apr 18, 2024
### What changes were proposed in this pull request? The pr aims to upgrade `netty` from `4.1.108.Final` to `4.1.109.Final`. ### Why are the changes needed? https://netty.io/news/2024/04/15/4-1-109-Final.html This version has brought some bug fixes and improvements, such as: - Fix DefaultChannelId#asLongText NPE ([#13971](netty/netty#13971)) - Rewrite ZstdDecoder to remove the need of allocate a huge byte[] internally ([#13928](netty/netty#13928)) - Don't send a RST frame when closing the stream in a write future while processing inbound frames ([#13973](netty/netty#13973)) ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #46112 from panbingkun/netty_for_spark4. Authored-by: panbingkun <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Utils.terminateProcess should
destroy()first and only fall back todestroyForcibly()if it fails. It's kind of bad that we're force-killing executors -- and only in Java 8. See JIRA for an example of the impact: no shutdownWhile here:
Utils.waitForProcessshould use the Java 8 method if available instead of a custom implementation.How was this patch tested?
Existing tests, which cover the force-kill case, and Amplab tests, which will cover both Java 7 and Java 8 eventually. However I tested locally on Java 8 and the PR builder will try Java 7 here.