You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by darabos <gi...@git.apache.org> on 2014/07/08 15:12:49 UTC
[GitHub] spark pull request: [SPARK-2403] Catch all errors during serializa...
GitHub user darabos opened a pull request:
https://github.com/apache/spark/pull/1329
[SPARK-2403] Catch all errors during serialization in DAGScheduler
https://issues.apache.org/jira/browse/SPARK-2403
Spark hangs for us whenever we forget to register a class with Kryo. This should be a simple fix for that. But let me know if you have a better suggestion.
I did not write a new test for this. It would be pretty complicated and I'm not sure it's worthwhile for such a simple change. Let me know if you disagree.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/darabos/spark spark-2403
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/1329.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1329
----
commit 361e96225bad6497009d75c74778336105854a6c
Author: Daniel Darabos <da...@gmail.com>
Date: 2014-07-08T13:07:30Z
Catch all errors during serialization in DAGScheduler.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2403] Catch all errors during serializa...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1329#issuecomment-48367185
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2403] Catch all errors during serializa...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1329#issuecomment-48375496
All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16409/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2403] Catch all errors during serializa...
Posted by darabos <gi...@git.apache.org>.
Github user darabos commented on a diff in the pull request:
https://github.com/apache/spark/pull/1329#discussion_r14665377
--- Diff: core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala ---
@@ -768,6 +768,10 @@ class DAGScheduler(
abortStage(stage, "Task not serializable: " + e.toString)
runningStages -= stage
return
+ case e: Throwable => // Other exceptions, such as IllegalArgumentException from Kryo.
--- End diff --
I suspect you are testing this on 2.10. Looks like a change in 2.11:
https://github.com/scala/scala/commit/64603653f82082431941cb29ad317b669822f28e#diff-ff42321ce198f97308744271b7e17c76
I think their argument applies to Spark too. Sounds like it is not safe to try and recover from StackOverflowError.
Thanks for the comments! I'll update the pull request in a moment.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2403] Catch all errors during serializa...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1329#issuecomment-48369146
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2403] Catch all errors during serializa...
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on the pull request:
https://github.com/apache/spark/pull/1329#issuecomment-48374016
Great, thanks! I just wanted to make sure it was actually printed somewhere, although I understand the behavior was not ideal.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2403] Catch all errors during serializa...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1329#issuecomment-48367175
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2403] Catch all errors during serializa...
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on a diff in the pull request:
https://github.com/apache/spark/pull/1329#discussion_r14664065
--- Diff: core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala ---
@@ -768,6 +768,10 @@ class DAGScheduler(
abortStage(stage, "Task not serializable: " + e.toString)
runningStages -= stage
return
+ case e: Throwable => // Other exceptions, such as IllegalArgumentException from Kryo.
+ abortStage(stage, "Task serialization failed: " + e.toString)
--- End diff --
In the prior case, of NotSerializableException, the error message was likely sufficient. However, since we're dealing with arbitrary exceptions here, the user may need to actually see the full stack trace to understand what's going on. Maybe something like:
```scala
abortStage(stage, s"Task serialization failed: $e\n${e.getStackTraceString}")
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2403] Catch all errors during serializa...
Posted by darabos <gi...@git.apache.org>.
Github user darabos commented on the pull request:
https://github.com/apache/spark/pull/1329#issuecomment-48368953
Thanks! I've added the suggested changes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2403] Catch all errors during serializa...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/1329
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2403] Catch all errors during serializa...
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on the pull request:
https://github.com/apache/spark/pull/1329#issuecomment-48370178
LGTM. Regarding the initial problem you observed, did you see the actual exception via the DAGScheduler's OneForOneStrategy failure? Or were there no log messages containing the error?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2403] Catch all errors during serializa...
Posted by darabos <gi...@git.apache.org>.
Github user darabos commented on the pull request:
https://github.com/apache/spark/pull/1329#issuecomment-48373710
> LGTM. Regarding the initial problem you observed, did you see the actual exception via the DAGScheduler's OneForOneStrategy failure? Or were there no log messages containing the error?
Yes, the exception was logged from OneForOneStrategy. See the stack trace in https://issues.apache.org/jira/browse/SPARK-2403. (Well, except I omitted the first line which names OneForOneStrategy. Sorry about that.)
But after logging that, the system stalled. `localhost:4040` was refusing connections and `jstack` showed just a number of waiting threads. (I've left work now, but I can paste more details tomorrow if you're interested.)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2403] Catch all errors during serializa...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1329#issuecomment-48375494
Merged build finished. All automated tests passed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2403] Catch all errors during serializa...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1329#issuecomment-48372857
All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16408/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2403] Catch all errors during serializa...
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on the pull request:
https://github.com/apache/spark/pull/1329#issuecomment-48374163
Merged into master and branch-1.0. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2403] Catch all errors during serializa...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1329#issuecomment-48369131
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2403] Catch all errors during serializa...
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on a diff in the pull request:
https://github.com/apache/spark/pull/1329#discussion_r14663412
--- Diff: core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala ---
@@ -768,6 +768,10 @@ class DAGScheduler(
abortStage(stage, "Task not serializable: " + e.toString)
runningStages -= stage
return
+ case e: Throwable => // Other exceptions, such as IllegalArgumentException from Kryo.
--- End diff --
Please catch [NonFatal(e)](http://www.scala-lang.org/files/archive/nightly/docs/library/index.html#scala.util.control.NonFatal$) instead. I think we should catch StackOverflowError here (as that is a possible error during serialization), but we should not catch OOMs and other such throwables except to re-throw them.
NB: Despite what the documentation says, NonFatal does indeed seem to catch StackOverflowError:
```scala
scala> NonFatal(new StackOverflowError())
res1: Boolean = true
scala> NonFatal(new OutOfMemoryError())
res2: Boolean = false
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2403] Catch all errors during serializa...
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on the pull request:
https://github.com/apache/spark/pull/1329#issuecomment-48366789
Jenkins, ok to test.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2403] Catch all errors during serializa...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1329#issuecomment-48372856
Merged build finished. All automated tests passed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-2403] Catch all errors during serializa...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1329#issuecomment-48333968
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---