Skip to content

[SPARK-3469] Make sure all TaskCompletionListener are called even with failures #2343

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
Closed
wants to merge 5 commits into from
Closed

[SPARK-3469] Make sure all TaskCompletionListener are called even with failures #2343

wants to merge 5 commits into from

Conversation

rxin
Copy link
Contributor

@rxin rxin commented Sep 10, 2014

This is necessary because we rely on this callback interface to clean resources up. The old behavior would lead to resource leaks.

Note that this also changes the fault semantics of TaskCompletionListener. Previously failures in TaskCompletionListeners would result in the task being reported immediately. With this change, we report the exception at the end, and the reported exception is a TaskCompletionListenerException that contains all the exception messages.

@rxin
[SPARK-3469] Call all TaskCompletionListeners even if some fail.
1cb444d 
Note that this also changes the fault semantics of TaskCompletionListener.
Previously failures in TaskCompletionListeners would result in the task
being reported as failed. With this change, tasks won't be reported as failed
simply because the execution of TaskCompletionListener fails.
@rxin
Copy link
Contributor Author

rxin commented Sep 10, 2014

Jenkins, add to whitelist.

@rxin
Copy link
Contributor Author

rxin commented Sep 10, 2014

Jenkins, test this please.

@SparkQA
Copy link

SparkQA commented Sep 11, 2014

QA tests have started for PR 2343 at commit 1cb444d.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 11, 2014

QA tests have finished for PR 2343 at commit 1cb444d.

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

@SparkQA
Copy link

SparkQA commented Sep 11, 2014

QA tests have started for PR 2343 at commit 1cb444d.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 11, 2014

QA tests have started for PR 2343 at commit 29b6162.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 11, 2014

QA tests have finished for PR 2343 at commit 1cb444d.

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

@SparkQA
Copy link

SparkQA commented Sep 11, 2014

QA tests have finished for PR 2343 at commit 29b6162.

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

@shivaram
Copy link
Contributor

Do we need to change the semantics though ? We could still save the first exception and return a task failure ?

@rxin
Copy link
Contributor Author

rxin commented Sep 12, 2014

What do you think would be a good way to handle this?

Perhaps throw back the first exception encountered? or throw an exception with all the error messages

@rxin rxin closed this Sep 12, 2014
@rxin rxin reopened this Sep 12, 2014
@rxin
Copy link
Contributor Author

rxin commented Sep 12, 2014

Ok I pushed a new version that creates a TaskCompletionListenerException that contains all the error messages.

@SparkQA
Copy link

SparkQA commented Sep 12, 2014

QA tests have started for PR 2343 at commit aa68ea4.

  • This patch merges cleanly.

@rxin rxin changed the title [SPARK-3469] Call all TaskCompletionListeners even if some fail [SPARK-3469] Make sure all TaskCompletionListener are called even with failures Sep 12, 2014
@SparkQA
Copy link

SparkQA commented Sep 12, 2014

QA tests have finished for PR 2343 at commit aa68ea4.

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

shivaram
shivaram reviewed Sep 12, 2014
View reviewed changes
core/src/main/scala/org/apache/spark/TaskContext.scala
@@ -77,6 +77,8 @@ class TaskContext(
/**
* Add a listener in the form of a Scala closure to be executed on task completion.
* This will be called in all situation - success, failure, or cancellation.
* Exceptions in callbacks are however not thrown back upstream, i.e. tasks won't marked as
* failed even if completion callbacks fail to execute.
Copy link
Contributor

Choose a reason for hiding this comment

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

update this comment

@shivaram
Copy link
Contributor

I left a minor comment - Otherwise LGTM

@shivaram
Copy link
Contributor

Jenkins, retest this please

@SparkQA
Copy link

SparkQA commented Sep 13, 2014

QA tests have started for PR 2343 at commit ac5baea.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 13, 2014

QA tests have finished for PR 2343 at commit ac5baea.

  • This patch passes unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • throw new IllegalStateException("The main method in the given main class must be static")
    • class TaskCompletionListenerException(errorMessages: Seq[String]) extends Exception

@rxin
Copy link
Contributor Author

rxin commented Sep 13, 2014

Ok merging this in master. Thanks for taking a look.

@asfgit asfgit closed this in 2584ea5 Sep 13, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rxin @SparkQA @shivaram