You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by brad-kaiser <gi...@git.apache.org> on 2017/11/28 19:57:56 UTC
[GitHub] spark pull request #19836: [SPARK-22618][CORE] Catch exception in removeRDD ...
GitHub user brad-kaiser opened a pull request:
https://github.com/apache/spark/pull/19836
[SPARK-22618][CORE] Catch exception in removeRDD to stop jobs from dying
## What changes were proposed in this pull request?
I propose that BlockManagerMasterEndpoint.removeRdd() should catch and log any IOExceptions it receives. As it is now, the exception can bubble up to the main thread and kill user applications when called from RDD.unpersist(). I think this change is a better experience for the end user.
I chose to catch the exception in BlockManagerMasterEndpoint.removeRdd() instead of RDD.unpersist() because this way the RDD.unpersist() blocking option will still work correctly. Otherwise, blocking will get short circuited by the first error.
## How was this patch tested?
This patch was tested with a job that shows the job killing behavior mentioned above.
@rxin, it looks like you originally wrote this method, I would appreciate it if you took a look. Thanks.
This contribution is my original work and is licensed under the project's open source license.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/brad-kaiser/spark catch-unpersist-exception
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/19836.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 #19836
----
commit 889993f67853555afa3f2413ff64d4e253f3c0ce
Author: Brad Kaiser <ka...@us.ibm.com>
Date: 2017-11-27T19:29:52Z
[SPARK-22618][CORE] Catch exception in removeRDD to stop jobs from dying
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19836: [SPARK-22618][CORE] Catch exception in removeRDD to stop...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19836
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19836: [SPARK-22618][CORE] Catch exception in removeRDD to stop...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19836
**[Test build #84553 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84553/testReport)** for PR 19836 at commit [`fbd2497`](https://github.com/apache/spark/commit/fbd2497dca4bed8724cffeac5fc4b1ee51f7cb36).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19836: [SPARK-22618][CORE] Catch exception in removeRDD to stop...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19836
**[Test build #84562 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84562/testReport)** for PR 19836 at commit [`e2ad8c3`](https://github.com/apache/spark/commit/e2ad8c3a58984d0cca0cc3c4d4894bd2f25c2eed).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19836: [SPARK-22618][CORE] Catch exception in removeRDD ...
Posted by brad-kaiser <gi...@git.apache.org>.
Github user brad-kaiser commented on a diff in the pull request:
https://github.com/apache/spark/pull/19836#discussion_r153686556
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManagerMasterEndpoint.scala ---
@@ -159,11 +160,18 @@ class BlockManagerMasterEndpoint(
// Ask the slaves to remove the RDD, and put the result in a sequence of Futures.
// The dispatcher is used as an implicit argument into the Future sequence construction.
val removeMsg = RemoveRdd(rddId)
- Future.sequence(
- blockManagerInfo.values.map { bm =>
- bm.slaveEndpoint.ask[Int](removeMsg)
- }.toSeq
- )
+
+ val handleRemoveRddException: PartialFunction[Throwable, Int] = {
+ case e: IOException =>
+ logError(s"Error trying to remove rdd $rddId", e)
--- End diff --
Thanks for looking at my change. I changed the log to warning and "rdd" to "RDD". I had pulled out the partial function out because I felt like the expression was getting too deeply nested and hard to read. I certainly don't have to do that though.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19836: [SPARK-22618][CORE] Catch exception in removeRDD ...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on a diff in the pull request:
https://github.com/apache/spark/pull/19836#discussion_r155407888
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManagerMasterEndpoint.scala ---
@@ -159,11 +160,16 @@ class BlockManagerMasterEndpoint(
// Ask the slaves to remove the RDD, and put the result in a sequence of Futures.
// The dispatcher is used as an implicit argument into the Future sequence construction.
val removeMsg = RemoveRdd(rddId)
- Future.sequence(
- blockManagerInfo.values.map { bm =>
- bm.slaveEndpoint.ask[Int](removeMsg)
- }.toSeq
- )
+
+ val futures = blockManagerInfo.values.map { bm =>
+ bm.slaveEndpoint.ask[Int](removeMsg).recover {
+ case e: IOException =>
--- End diff --
Sounds good, thanks!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19836: [SPARK-22618][CORE] Catch exception in removeRDD to stop...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19836
LGTM
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19836: [SPARK-22618][CORE] Catch exception in removeRDD ...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/19836#discussion_r153641688
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManagerMasterEndpoint.scala ---
@@ -159,11 +160,18 @@ class BlockManagerMasterEndpoint(
// Ask the slaves to remove the RDD, and put the result in a sequence of Futures.
// The dispatcher is used as an implicit argument into the Future sequence construction.
val removeMsg = RemoveRdd(rddId)
- Future.sequence(
- blockManagerInfo.values.map { bm =>
- bm.slaveEndpoint.ask[Int](removeMsg)
- }.toSeq
- )
+
+ val handleRemoveRddException: PartialFunction[Throwable, Int] = {
+ case e: IOException =>
+ logError(s"Error trying to remove rdd $rddId", e)
--- End diff --
Warning, not error, I'd imagine. rdd -> RDD. Why this construction with a partial function rather than write this inline below?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19836: [SPARK-22618][CORE] Catch exception in removeRDD ...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/19836#discussion_r155230290
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManagerMasterEndpoint.scala ---
@@ -159,11 +160,18 @@ class BlockManagerMasterEndpoint(
// Ask the slaves to remove the RDD, and put the result in a sequence of Futures.
// The dispatcher is used as an implicit argument into the Future sequence construction.
val removeMsg = RemoveRdd(rddId)
- Future.sequence(
- blockManagerInfo.values.map { bm =>
- bm.slaveEndpoint.ask[Int](removeMsg)
- }.toSeq
- )
+
+ val handleRemoveRddException: PartialFunction[Throwable, Int] = {
+ case e: IOException =>
+ logWarning(s"Error trying to remove RDD $rddId", e)
+ 0 // zero blocks were removed
+ }
+
+ val futures = blockManagerInfo.values.map { bm =>
+ bm.slaveEndpoint.ask[Int](removeMsg).recover(handleRemoveRddException)
--- End diff --
personally I think
```
bm.slaveEndpoint.ask[Int](removeMsg).recover {
case e: IOException =>
logWarning(s"Error trying to remove RDD $rddId", e)
0 // zero blocks were removed
}
```
is more readable
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19836: [SPARK-22618][CORE] Catch exception in removeRDD ...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on a diff in the pull request:
https://github.com/apache/spark/pull/19836#discussion_r155265124
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManagerMasterEndpoint.scala ---
@@ -159,11 +160,16 @@ class BlockManagerMasterEndpoint(
// Ask the slaves to remove the RDD, and put the result in a sequence of Futures.
// The dispatcher is used as an implicit argument into the Future sequence construction.
val removeMsg = RemoveRdd(rddId)
- Future.sequence(
- blockManagerInfo.values.map { bm =>
- bm.slaveEndpoint.ask[Int](removeMsg)
- }.toSeq
- )
+
+ val futures = blockManagerInfo.values.map { bm =>
+ bm.slaveEndpoint.ask[Int](removeMsg).recover {
+ case e: IOException =>
--- End diff --
According to what is described in the JIRA, should we only ignore the `IOException` if dynamic allocation is enabled?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19836: [SPARK-22618][CORE] Catch exception in removeRDD to stop...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19836
ok to test
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19836: [SPARK-22618][CORE] Catch exception in removeRDD to stop...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19836
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84562/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19836: [SPARK-22618][CORE] Catch exception in removeRDD to stop...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19836
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19836: [SPARK-22618][CORE] Catch exception in removeRDD to stop...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19836
**[Test build #84553 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84553/testReport)** for PR 19836 at commit [`fbd2497`](https://github.com/apache/spark/commit/fbd2497dca4bed8724cffeac5fc4b1ee51f7cb36).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19836: [SPARK-22618][CORE] Catch exception in removeRDD to stop...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19836
thanks, merging to master!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19836: [SPARK-22618][CORE] Catch exception in removeRDD to stop...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19836
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19836: [SPARK-22618][CORE] Catch exception in removeRDD to stop...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19836
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84553/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19836: [SPARK-22618][CORE] Catch exception in removeRDD ...
Posted by brad-kaiser <gi...@git.apache.org>.
Github user brad-kaiser commented on a diff in the pull request:
https://github.com/apache/spark/pull/19836#discussion_r155267282
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManagerMasterEndpoint.scala ---
@@ -159,11 +160,16 @@ class BlockManagerMasterEndpoint(
// Ask the slaves to remove the RDD, and put the result in a sequence of Futures.
// The dispatcher is used as an implicit argument into the Future sequence construction.
val removeMsg = RemoveRdd(rddId)
- Future.sequence(
- blockManagerInfo.values.map { bm =>
- bm.slaveEndpoint.ask[Int](removeMsg)
- }.toSeq
- )
+
+ val futures = blockManagerInfo.values.map { bm =>
+ bm.slaveEndpoint.ask[Int](removeMsg).recover {
+ case e: IOException =>
--- End diff --
I think the logic for catching the error still applies even without dynamic allocation. If one of your nodes goes down while you happen to be in .unpersist, you wouldn't want your whole job to fail.
Dynamic allocation just makes this scenario more likely.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19836: [SPARK-22618][CORE] Catch exception in removeRDD to stop...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on the issue:
https://github.com/apache/spark/pull/19836
This looks reasonable, cc @cloud-fan
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19836: [SPARK-22618][CORE] Catch exception in removeRDD to stop...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19836
**[Test build #84562 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84562/testReport)** for PR 19836 at commit [`e2ad8c3`](https://github.com/apache/spark/commit/e2ad8c3a58984d0cca0cc3c4d4894bd2f25c2eed).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19836: [SPARK-22618][CORE] Catch exception in removeRDD ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/19836
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19836: [SPARK-22618][CORE] Catch exception in removeRDD ...
Posted by brad-kaiser <gi...@git.apache.org>.
Github user brad-kaiser commented on a diff in the pull request:
https://github.com/apache/spark/pull/19836#discussion_r155255972
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManagerMasterEndpoint.scala ---
@@ -159,11 +160,18 @@ class BlockManagerMasterEndpoint(
// Ask the slaves to remove the RDD, and put the result in a sequence of Futures.
// The dispatcher is used as an implicit argument into the Future sequence construction.
val removeMsg = RemoveRdd(rddId)
- Future.sequence(
- blockManagerInfo.values.map { bm =>
- bm.slaveEndpoint.ask[Int](removeMsg)
- }.toSeq
- )
+
+ val handleRemoveRddException: PartialFunction[Throwable, Int] = {
+ case e: IOException =>
+ logWarning(s"Error trying to remove RDD $rddId", e)
+ 0 // zero blocks were removed
+ }
+
+ val futures = blockManagerInfo.values.map { bm =>
+ bm.slaveEndpoint.ask[Int](removeMsg).recover(handleRemoveRddException)
--- End diff --
Ok, I updated this. Thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org