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-6014] [core] Revamp Spark shutdown hooks, fix shutdown races. #5560

Closed
wants to merge 3 commits into from

Conversation

vanzin
Copy link
Contributor

@vanzin vanzin commented Apr 17, 2015

This change adds some new utility code to handle shutdown hooks in
Spark. The main goal is to take advantage of Hadoop 2.x's API for
shutdown hooks, which allows Spark to register a hook that will
run before the one that cleans up HDFS clients, and thus avoids
some races that would cause exceptions to show up and other issues
such as failure to properly close event logs.

Unfortunately, Hadoop 1.x does not have such APIs, so in that case
correctness is still left to chance.

This change adds some new utility code to handle shutdown hooks in
Spark. The main goal is to take advantage of Hadoop 2.x's API for
shutdown hooks, which allows Spark to register a hook that will
run before the one that cleans up HDFS clients, and thus avoids
some races that would cause exceptions to show up and other issues
such as failure to properly close event logs.

Unfortunately, Hadoop 1.x does not have such APIs, so in that case
correctness is still left to chance.
@SparkQA
Copy link

SparkQA commented Apr 17, 2015

Test build #30494 has finished for PR 5560 at commit e7039dc.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

/**
* Adds a shutdown hook with default priority.
*/
def addShutdownHook(hook: () => Unit): AnyRef = {
Copy link
Member

Choose a reason for hiding this comment

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

It looked weird to return AnyRef, but I suppose that while callers need a handle to the hook, they don't need to be promised anything about what it is.

@srowen
Copy link
Member

srowen commented Apr 18, 2015

Yep, I like this. It cleans up a kind of gnarly aspect and centralizes handling cleanly. It integrates with a similar mechanism in Hadoop if it exists, and that in turn fixes some actual small problems we encounter at shutdown.

Needs a rebase though.

Marcelo Vanzin added 2 commits April 20, 2015 11:11
Conflicts:
	yarn/src/main/scala/org/apache/spark/deploy/yarn/ApplicationMaster.scala
@SparkQA
Copy link

SparkQA commented Apr 20, 2015

Test build #30598 has finished for PR 5560 at commit edfafb1.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • public class MapConfigProvider extends ConfigProvider
  • This patch does not change any dependencies.

@asfgit asfgit closed this in e72c16e Apr 22, 2015
@vanzin vanzin deleted the SPARK-6014 branch April 22, 2015 00:39
nemccarthy pushed a commit to nemccarthy/spark that referenced this pull request Jun 19, 2015
This change adds some new utility code to handle shutdown hooks in
Spark. The main goal is to take advantage of Hadoop 2.x's API for
shutdown hooks, which allows Spark to register a hook that will
run before the one that cleans up HDFS clients, and thus avoids
some races that would cause exceptions to show up and other issues
such as failure to properly close event logs.

Unfortunately, Hadoop 1.x does not have such APIs, so in that case
correctness is still left to chance.

Author: Marcelo Vanzin <vanzin@cloudera.com>

Closes apache#5560 from vanzin/SPARK-6014 and squashes the following commits:

edfafb1 [Marcelo Vanzin] Better scaladoc.
fcaeedd [Marcelo Vanzin] Merge branch 'master' into SPARK-6014
e7039dc [Marcelo Vanzin] [SPARK-6014] [core] Revamp Spark shutdown hooks, fix shutdown races.
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.

3 participants