You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by YanTangZhai <gi...@git.apache.org> on 2014/07/01 12:43:44 UTC
[GitHub] spark pull request: [SPARK-2324] SparkContext should not exit dire...
GitHub user YanTangZhai opened a pull request:
https://github.com/apache/spark/pull/1274
[SPARK-2324] SparkContext should not exit directly when spark.local.dir is a list of multiple paths and one of them has error
The spark.local.dir is configured as a list of multiple paths as follows /data1/sparkenv/local,/data2/sparkenv/local. If the disk data2 of the driver node has error, the application will exit since DiskBlockManager exits directly at createLocalDirs. If the disk data2 of the worker node has error, the executor will exit either.
DiskBlockManager should not exit directly at createLocalDirs if one of spark.local.dir has error. Since spark.local.dir has multiple paths, a problem should not affect the overall situation.
I think DiskBlockManager could ignore the bad directory at createLocalDirs.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/YanTangZhai/spark SPARK-2324
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/1274.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 #1274
----
commit df086731952c669e12673fd673d829b9fdd790a2
Author: yantangzhai <ty...@163.com>
Date: 2014-07-01T10:39:46Z
[SPARK-2324] SparkContext should not exit directly when spark.local.dir is a list of multiple paths and one of them has 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-2324] SparkContext should not exit dire...
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on a diff in the pull request:
https://github.com/apache/spark/pull/1274#discussion_r14418731
--- Diff: core/src/main/scala/org/apache/spark/storage/DiskBlockManager.scala ---
@@ -115,8 +121,9 @@ private[spark] class DiskBlockManager(shuffleManager: ShuffleBlockManager, rootD
private def createLocalDirs(): Array[File] = {
logDebug(s"Creating local directories at root dirs '$rootDirs'")
+ val localDirsResult = ArrayBuffer[File]()
val dateFormat = new SimpleDateFormat("yyyyMMddHHmmss")
- rootDirs.split(",").map { rootDir =>
+ rootDirs.split(",").foreach { rootDir =>
--- End diff --
Scala style thing, you can use flatMap instead of foreach here and return None in the case where directory creation failed and Some(localDir) in the case where it worked.
---
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-2324] SparkContext should not exit dire...
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on the pull request:
https://github.com/apache/spark/pull/1274#issuecomment-47690237
This change seems reasonable because on large clusters, we occasionally see a single disk on a single machine is failed, and this may cause the entire application to crash because the executor will keep getting restarted until the Master kills the application.
It also allows a more uniform configuration for heterogeneous cluster with different numbers of disks.
The downside of this behavioral change is that a misconfiguration like mistyping one of your local dirs may go unnoticed for a while, but this will hopefully become apparent after a `df` or a look at any of the executor logs. This fail-fast approach is generally better, but current Spark does not do a good job communicating the reason for executors that crash immediately upon startup.
---
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-2324] SparkContext should not exit dire...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/1274
---
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-2324] SparkContext should not exit dire...
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on the pull request:
https://github.com/apache/spark/pull/1274#issuecomment-47689007
Jenkins, ok to 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-2324] SparkContext should not exit dire...
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on a diff in the pull request:
https://github.com/apache/spark/pull/1274#discussion_r14418861
--- Diff: core/src/main/scala/org/apache/spark/storage/DiskBlockManager.scala ---
@@ -137,11 +144,12 @@ private[spark] class DiskBlockManager(shuffleManager: ShuffleBlockManager, rootD
}
if (!foundLocalDir) {
logError(s"Failed $MAX_DIR_CREATION_ATTEMPTS attempts to create local dir in $rootDir")
--- End diff --
Maybe add to this log to say that you are ignoring this directory moving forward. e.g., something simple like,
```scala
logError(s"Failed $MAX_DIR_CREATION_ATTEMPTS attempts to create local dir in $rootDir."
+ " Ignoring this directory.")
```
---
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-2324] SparkContext should not exit dire...
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on a diff in the pull request:
https://github.com/apache/spark/pull/1274#discussion_r14418939
--- Diff: core/src/main/scala/org/apache/spark/storage/DiskBlockManager.scala ---
@@ -26,6 +26,8 @@ import org.apache.spark.executor.ExecutorExitCode
import org.apache.spark.network.netty.{PathResolver, ShuffleSender}
import org.apache.spark.util.Utils
+import scala.collection.mutable.ArrayBuffer
--- End diff --
nit: this "scala.*" import should go into their own block in between the java.* and org.* imports. See our [style guide](https://cwiki.apache.org/confluence/display/SPARK/Spark+Code+Style+Guide#SparkCodeStyleGuide-Imports).
---
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-2324] SparkContext should not exit dire...
Posted by YanTangZhai <gi...@git.apache.org>.
Github user YanTangZhai commented on the pull request:
https://github.com/apache/spark/pull/1274#issuecomment-47737851
Thank aarondav. I've modified some codes. Please help to review again.
---
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-2324] SparkContext should not exit dire...
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on the pull request:
https://github.com/apache/spark/pull/1274#issuecomment-47958114
LGTM. Merging into master.
---
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-2324] SparkContext should not exit dire...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/1274#issuecomment-51537965
@aarondav @YanTangZhai I think that this patch introduced a bug in `Utils.getLocalDir()`. Currently, that function assumes that all of the directories in `spark.local.dir` exist and returns the first directory from that list:
```scala
/**
* Get a temporary directory using Spark's spark.local.dir property, if set. This will always
* return a single directory, even though the spark.local.dir property might be a list of
* multiple paths.
*/
def getLocalDir(conf: SparkConf): String = {
conf.get("spark.local.dir", System.getProperty("java.io.tmpdir")).split(',')(0)
}
```
After this patch, the first directory might be missing, which can lead to confusing errors when we try to create files in it.
How should we fix this? Maybe have the disk manager be the authoritative source of local directories and update all other code to use it rather than the raw `spark.local.dir` property?
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-2324] SparkContext should not exit dire...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1274#issuecomment-47641772
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.
---