You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by jiangxb1987 <gi...@git.apache.org> on 2018/05/22 07:13:40 UTC
[GitHub] spark pull request #21390: [SPARK-24340][Core] Clean up non-shuffle disk blo...
GitHub user jiangxb1987 opened a pull request:
https://github.com/apache/spark/pull/21390
[SPARK-24340][Core] Clean up non-shuffle disk block manager files following executor death
## What changes were proposed in this pull request?
Currently we only clean up the local directories on application removed. However, when executors die and restart repeatedly, many temp files are left untouched in the local directories, which is undesired behavior and could cause disk space used up gradually.
We can detect executor death in the Worker, and clean up the non-shuffle files (files not ended with ".index" or ".data") in the local directories, we should not touch the shuffle files since they are expected to be used by the external shuffle service.
## How was this patch tested?
Add new test suite to cover.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jiangxb1987/spark cleanupNonshuffleFiles
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/21390.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 #21390
----
commit 251726ec66805804c12d5ed9caf4fa84bfe8a298
Author: Xingbo Jiang <xi...@...>
Date: 2018-05-08T15:29:03Z
implement delete non-shuffle files on executor finished.
commit 2ba90a237bbb013d913eb8aceffe41ee49ec1ca0
Author: Xingbo Jiang <xi...@...>
Date: 2018-05-10T17:26:45Z
refactor
commit 64bde5f43a3a4e64f8ce5d69f03997ca10508431
Author: Xingbo Jiang <xi...@...>
Date: 2018-05-21T13:16:07Z
add test suite
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on the issue:
https://github.com/apache/spark/pull/21390
YARN will clean container local dirs when container (executor) is exited, so this may not be a problem in YARN.
YARN has a useful configuration "yarn.nodemanager.delete.debug-delay-sec" to delay the container dir cleanup for a specified time, which is quite useful for debug. Maybe we can add a similar config here?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21390
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/3521/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21390
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/91006/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21390
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 #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the issue:
https://github.com/apache/spark/pull/21390
Yeah, this is only concerned with non-shuffle files which are located in the block manager temp directories (e.g. large sorter spill files).
There is a related issue where shuffle files can be leaked indefinitely following executor death because the external shuffle service is never directly told that shuffles are safe to remove (the context cleaner sends RPCs to executors and executors clean up their own shuffle files). That issue is substantially harder to fix, though, since it likely requires protocol changes to the shuffle service or an inversion-of-control where the shuffle service can periodically ask the driver "do any of these shuffle IDs correspond to cleaned shuffles?". As a result, I think the strategy here is to decompose that disk leak into two separate sets of fixes, where this patch is concerned with the simpler case of non-shuffle files (we'll defer the more complex case to a separate PR because it requires a lot more design).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21390: [SPARK-24340][Core] Clean up non-shuffle disk blo...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on a diff in the pull request:
https://github.com/apache/spark/pull/21390#discussion_r190571272
--- Diff: core/src/main/scala/org/apache/spark/deploy/worker/Worker.scala ---
@@ -97,6 +99,10 @@ private[deploy] class Worker(
private val APP_DATA_RETENTION_SECONDS =
conf.getLong("spark.worker.cleanup.appDataTtl", 7 * 24 * 3600)
+ // Whether or not cleanup the non-shuffle files on executor death.
+ private val CLEANUP_NON_SHUFFLE_FILES_ENABLED =
+ conf.getBoolean("spark.storage.cleanupFilesAfterExecutorDeath", true)
--- End diff --
Shall we rename this config to "spark.storage.cleanupFilesAfterExecutorExit"? Seems from the code that normal executor exit (dynamic allocation) will also trigger the cleanup, this config may be a little misleading. Please correct me if I'm wrong.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21390
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21390
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 #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21390
**[Test build #91064 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/91064/testReport)** for PR 21390 at commit [`0df8e4e`](https://github.com/apache/spark/commit/0df8e4ec71971468854b6d778a1899df8df71211).
* 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 #21390: [SPARK-24340][Core] Clean up non-shuffle disk blo...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/21390#discussion_r189813156
--- Diff: core/src/main/scala/org/apache/spark/deploy/worker/Worker.scala ---
@@ -732,6 +736,9 @@ private[deploy] class Worker(
trimFinishedExecutorsIfNecessary()
coresUsed -= executor.cores
memoryUsed -= executor.memory
+ if (CLEANUP_NON_SHUFFLE_FILES_ENABLED) {
+ shuffleService.executorRemoved(executorStateChanged.execId.toString, appId)
--- End diff --
Re-reading this code with fresh eyes made me realize that maybe the method naming here is a bit confusing: from the name alone, it sounds like `executorRemoved` is a notification-sending method, the kind of thing where you'd always want to call it when the corresponding event (executor removal) occurs and then leave it up to the method implementation to decide whether to actually carry out specific cleanup logic.
On the other hand, we call `shuffleService.applicationRemoved()` in `maybeCleanupApplication()` and there's a flag in that method controlling whether we actually clean up, so we have a similar pattern there.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21390
**[Test build #91006 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/91006/testReport)** for PR 21390 at commit [`0df8e4e`](https://github.com/apache/spark/commit/0df8e4ec71971468854b6d778a1899df8df71211).
* This patch **fails Spark unit 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 #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21390
**[Test build #90942 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/90942/testReport)** for PR 21390 at commit [`64bde5f`](https://github.com/apache/spark/commit/64bde5f43a3a4e64f8ce5d69f03997ca10508431).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21390: [SPARK-24340][Core] Clean up non-shuffle disk blo...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on a diff in the pull request:
https://github.com/apache/spark/pull/21390#discussion_r190098033
--- Diff: core/src/main/scala/org/apache/spark/deploy/worker/Worker.scala ---
@@ -97,6 +97,10 @@ private[deploy] class Worker(
private val APP_DATA_RETENTION_SECONDS =
conf.getLong("spark.worker.cleanup.appDataTtl", 7 * 24 * 3600)
+ // Whether or not cleanup the non-shuffle files on executor finishes.
+ private val CLEANUP_NON_SHUFFLE_FILES_ENABLED =
+ conf.getBoolean("spark.worker.cleanup.nonShuffleFiles.enabled", true)
--- End diff --
Is it possible that a user want to cleanup non-shuffle files but don't want to cleanup the whole application directories(may still make use of the shuffle data)? For this specific use case, we should not make `spark.worker.cleanup.enabled = true` a precondition of `spark.worker.cleanup.nonShuffleFiles.enabled = true`. Otherwise, I would be glad to make the change.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21390
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 pull request #21390: [SPARK-24340][Core] Clean up non-shuffle disk blo...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/21390
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21390
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/91064/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/21390
retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21390: [SPARK-24340][Core] Clean up non-shuffle disk blo...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/21390#discussion_r189813626
--- Diff: core/src/main/scala/org/apache/spark/deploy/worker/Worker.scala ---
@@ -97,6 +97,10 @@ private[deploy] class Worker(
private val APP_DATA_RETENTION_SECONDS =
conf.getLong("spark.worker.cleanup.appDataTtl", 7 * 24 * 3600)
+ // Whether or not cleanup the non-shuffle files on executor finishes.
+ private val CLEANUP_NON_SHUFFLE_FILES_ENABLED =
+ conf.getBoolean("spark.worker.cleanup.nonShuffleFiles.enabled", true)
--- End diff --
Is there potential confusion from the fact that `spark.worker.cleanup.nonShuffleFiles.enabled`'s effects are not controlled by `spark.worker.cleanup.enabled`? Should they be? The `spark.worker.cleanup.enabled` configuration only seems to be dealing with the cleanup of completed applications' directories, whereas this is dealing with cleanup from completed executors whose application continues running (likely with new executors).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21390
**[Test build #90942 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/90942/testReport)** for PR 21390 at commit [`64bde5f`](https://github.com/apache/spark/commit/64bde5f43a3a4e64f8ce5d69f03997ca10508431).
* This patch **fails Spark unit 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 #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21390
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21390
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/3533/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21390
**[Test build #91083 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/91083/testReport)** for PR 21390 at commit [`4a4ab59`](https://github.com/apache/spark/commit/4a4ab595a32537bd5ad022ec77f3e598a252a8ed).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on the issue:
https://github.com/apache/spark/pull/21390
For Kubernetes, without a story around the external shuffle service, all the scratch space used by executors will be cleaned up by Kubernetes itself. When we want shuffle data to persist across executor restarts we'll have to think about this matter more carefully.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21390
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/3448/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21390
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/91078/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21390
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/90942/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21390: [SPARK-24340][Core] Clean up non-shuffle disk blo...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/21390#discussion_r189809055
--- Diff: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolver.java ---
@@ -226,6 +246,29 @@ private void deleteExecutorDirs(String[] dirs) {
}
}
+ private FilenameFilter filter = new FilenameFilter() {
--- End diff --
Is it feasible to define this as a locale variable inside of `deleteNonShuffleFiles` in order to reduce its scope to be as small as possible and make it clear that this is only used within `deleteNonShuffleFiles`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21390
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/3536/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21390
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 #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the issue:
https://github.com/apache/spark/pull/21390
sounds like we need to have some sort of lifetime management, TTL -like design shuffle file - should we have a new JIRA on that?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21390
**[Test build #91078 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/91078/testReport)** for PR 21390 at commit [`4a4ab59`](https://github.com/apache/spark/commit/4a4ab595a32537bd5ad022ec77f3e598a252a8ed).
* This patch **fails Spark unit 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 #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on the issue:
https://github.com/apache/spark/pull/21390
Actually since this specifically applies to _non_-shuffle files I think Kubernetes will be fine here regardless.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21390
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 #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21390
**[Test build #91064 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/91064/testReport)** for PR 21390 at commit [`0df8e4e`](https://github.com/apache/spark/commit/0df8e4ec71971468854b6d778a1899df8df71211).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on the issue:
https://github.com/apache/spark/pull/21390
Are there any other concerns over this PR?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21390: [SPARK-24340][Core] Clean up non-shuffle disk blo...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/21390#discussion_r190104118
--- Diff: core/src/main/scala/org/apache/spark/deploy/worker/Worker.scala ---
@@ -732,6 +736,9 @@ private[deploy] class Worker(
trimFinishedExecutorsIfNecessary()
coresUsed -= executor.cores
memoryUsed -= executor.memory
+ if (CLEANUP_NON_SHUFFLE_FILES_ENABLED) {
+ shuffleService.executorRemoved(executorStateChanged.execId.toString, appId)
--- End diff --
Nah, no need to rename since this matches existing convention. I was just thinking aloud here / explaining the reasoning for other future readers.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/21390
Thanks! Merged to master.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21390: [SPARK-24340][Core] Clean up non-shuffle disk blo...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/21390#discussion_r189816085
--- Diff: common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/NonShuffleFilesCleanupSuite.java ---
@@ -0,0 +1,222 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.network.shuffle;
+
+import java.io.File;
+import java.io.FilenameFilter;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.util.Random;
+import java.util.concurrent.Executor;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import com.google.common.util.concurrent.MoreExecutors;
+import org.junit.Test;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import org.apache.spark.network.util.MapConfigProvider;
+import org.apache.spark.network.util.TransportConf;
+
+public class NonShuffleFilesCleanupSuite {
--- End diff --
Do we need a test to check that we preserve the old behavior in case the new configuration is set to `false`? An end-to-end test will likely be prone to flakiness, so instead maybe we could somehow test that `shuffleService. executorRemoved()` is _not_ called if the configuration is `false`. One way to do that would be to move the construction of `new ExternalShuffleService` from the default constructor of `Worker` into its public constructor and then inject it in the `new Worker` call. This, in turn, would let you inject either a mock or spy in order to verify call counts. Do you know if we have this style of test for other `Worker` functionality? Is this a ton of work or is it relatively simple to do?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21390
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 #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21390
**[Test build #91078 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/91078/testReport)** for PR 21390 at commit [`4a4ab59`](https://github.com/apache/spark/commit/4a4ab595a32537bd5ad022ec77f3e598a252a8ed).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on the issue:
https://github.com/apache/spark/pull/21390
@jerryshao Agree it should be useful to add a `debug-delay-sec` config for ease of developing, since this PR has already bring in a brunch of code changes, maybe we can add the config in a followup PR?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21390
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/3565/
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 #21390: [SPARK-24340][Core] Clean up non-shuffle disk blo...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/21390#discussion_r189809797
--- Diff: core/src/main/scala/org/apache/spark/deploy/worker/Worker.scala ---
@@ -97,6 +97,10 @@ private[deploy] class Worker(
private val APP_DATA_RETENTION_SECONDS =
conf.getLong("spark.worker.cleanup.appDataTtl", 7 * 24 * 3600)
+ // Whether or not cleanup the non-shuffle files on executor finishes.
+ private val CLEANUP_NON_SHUFFLE_FILES_ENABLED =
+ conf.getBoolean("spark.worker.cleanup.nonShuffleFiles.enabled", true)
--- End diff --
Should we document this configuration at https://github.com/apache/spark/blob/master/docs/spark-standalone.md ?
I can't imagine a great reason for users wanting to disable this and believe that this is functioning primarily as a feature-flag to give us an escape-hatch in case of regressions, so maybe it's not a huge deal to leave it undocumented?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21390
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21390
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/91083/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21390: [SPARK-24340][Core] Clean up non-shuffle disk blo...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on a diff in the pull request:
https://github.com/apache/spark/pull/21390#discussion_r190074813
--- Diff: common/network-common/src/main/java/org/apache/spark/network/util/JavaUtils.java ---
@@ -157,10 +172,10 @@ private static void deleteRecursivelyUsingUnixNative(File file) throws IOExcepti
}
}
- private static File[] listFilesSafely(File file) throws IOException {
+ private static File[] listFilesSafely(File file, FilenameFilter filter) throws IOException {
if (file.exists()) {
- File[] files = file.listFiles();
- if (files == null) {
+ File[] files = file.listFiles(filter);
+ if (files == null && filter == null) {
--- End diff --
Agreed, null is reserved for errors, and it's possible that `listFiles(filter)` returns null when the list operation fail.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21390: [SPARK-24340][Core] Clean up non-shuffle disk blo...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on a diff in the pull request:
https://github.com/apache/spark/pull/21390#discussion_r190098624
--- Diff: core/src/main/scala/org/apache/spark/deploy/worker/Worker.scala ---
@@ -732,6 +736,9 @@ private[deploy] class Worker(
trimFinishedExecutorsIfNecessary()
coresUsed -= executor.cores
memoryUsed -= executor.memory
+ if (CLEANUP_NON_SHUFFLE_FILES_ENABLED) {
+ shuffleService.executorRemoved(executorStateChanged.execId.toString, appId)
--- End diff --
It takes me great pain to name a function, config, etc, so I just followed the existing similar pattern. Thus, I'm happy to rename them, perhaps `cleanupNonshuffleFilesOfExecutor`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21390: [SPARK-24340][Core] Clean up non-shuffle disk blo...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on a diff in the pull request:
https://github.com/apache/spark/pull/21390#discussion_r190098764
--- Diff: common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/NonShuffleFilesCleanupSuite.java ---
@@ -0,0 +1,222 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.network.shuffle;
+
+import java.io.File;
+import java.io.FilenameFilter;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.util.Random;
+import java.util.concurrent.Executor;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import com.google.common.util.concurrent.MoreExecutors;
+import org.junit.Test;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import org.apache.spark.network.util.MapConfigProvider;
+import org.apache.spark.network.util.TransportConf;
+
+public class NonShuffleFilesCleanupSuite {
--- End diff --
Thanks for the suggestion, Let me try it later. :)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21390
**[Test build #91128 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/91128/testReport)** for PR 21390 at commit [`2011eed`](https://github.com/apache/spark/commit/2011eede002664ef75e00f1f0228c5d765753f4c).
* 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 #21390: [SPARK-24340][Core] Clean up non-shuffle disk blo...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/21390#discussion_r189808705
--- Diff: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolver.java ---
@@ -211,6 +211,26 @@ public void applicationRemoved(String appId, boolean cleanupLocalDirs) {
}
}
+ /**
+ * Removes all the non-shuffle files in any local directories associated with the finished
+ * executor.
+ */
+ public void executorRemoved(String executorId, String appId) {
+ logger.info("Clean up non-shuffle files associated with the finished executor {}", executorId);
+ AppExecId fullId = new AppExecId(appId, executorId);
+ final ExecutorShuffleInfo executor = executors.get(fullId);
+ if (executor == null) {
+ // Executor not registered, skip clean up of the local directories.
+ logger.info("Executor is not registered (appId={}, execId={})", appId, executorId);
+ } else {
+ logger.info("Cleaning up non-shuffle files in executor {}'s {} local dirs", fullId,
+ executor.localDirs.length);
+
+ // Execute the actual deletion in a different thread, as it may take some time.
--- End diff --
👍 , thanks for doing this in a separate thread. This indeed could take a _very_ long time under certain rare conditions.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21390
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 pull request #21390: [SPARK-24340][Core] Clean up non-shuffle disk blo...
Posted by mswit-databricks <gi...@git.apache.org>.
Github user mswit-databricks commented on a diff in the pull request:
https://github.com/apache/spark/pull/21390#discussion_r189857237
--- Diff: common/network-common/src/main/java/org/apache/spark/network/util/JavaUtils.java ---
@@ -157,10 +172,10 @@ private static void deleteRecursivelyUsingUnixNative(File file) throws IOExcepti
}
}
- private static File[] listFilesSafely(File file) throws IOException {
+ private static File[] listFilesSafely(File file, FilenameFilter filter) throws IOException {
if (file.exists()) {
- File[] files = file.listFiles();
- if (files == null) {
+ File[] files = file.listFiles(filter);
+ if (files == null && filter == null) {
--- End diff --
Are you sure that `lisFiles(FileFilter filter)` (with a non-null filter) returns a null when there are no hits? The documentation isn't clear, but if it was to be consistent with the zero-parameter `listFiles()`, it would return an empty array. null should be reserved for errors.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on the issue:
https://github.com/apache/spark/pull/21390
retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21390
**[Test build #91006 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/91006/testReport)** for PR 21390 at commit [`0df8e4e`](https://github.com/apache/spark/commit/0df8e4ec71971468854b6d778a1899df8df71211).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the issue:
https://github.com/apache/spark/pull/21390
Context for other reviewers: the issue addressed by this patch is actually a real issue in practice, especially for long-lived Spark clusters; I have seen this specific problem play a large contributing role to certain production out-of-disk-space failures.
One thing I'd like to note: as implemented here, this patch only addresses this problem for Spark's built-in "Standalone" cluster manager. @jiangxb1987, could you mention that limitation in the PR title and description? My personal preference is to proceed incrementally by merging this Standalone-only PR and and deferring support for other cluster managers to future PRs (perhaps from experts familiar with those other cluster managers).
I'll take a more detailed look tomorrow, but just wanted to provide motivation for other reviewers who might leave comments before then.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the issue:
https://github.com/apache/spark/pull/21390
Feel free to do the TTL in a followup. My feeling is that it won't be super useful in practice, though:
1. Cleanup of non-shuffle disk block manager files following executor exit only really matters for super-long-running applications. For short-running applications, you can just remove the entire application directory via the existing TTL cleaner mechanism.
2. If production jobs would fail with this change due to user code relying on undocumented internal behavior then I think the right solution is to disable this cleanup completely vs. putting it on a TTL. We've tried TTL-based cleanup before in the predecessor to the ContextCleaner and it was a huge source of user issues / JIRA tickets in cases where the cleanup was happening too soon (but not immediately, e.g. a 20 minute delay).
3. If you want this feature only for debugging (e.g. manual inspection of the contents of spill files) then I again image that you probably want an infinite timeout. Let's say I have a hard-to-reproduce production failure and I'd like to debug from the production repro by looking at spill files. In that case, the problem could occur at any hour, possibly when I'm asleep, so if I want the files to stick around long enough for a human to look at them then that could be several hours (possibly days in case we're running something over a weekend) and I feel like at a certain point a large timeout might as well become infinite.
Feel free to push back if you have a concrete use case where TTL-based cleanup of this specific file category is preferable to the binary on/off option implemented here. I'm just worried that it will be a lot of additional work to implement and will be harder to reason about (while offering relatively little additional marginal benefit compared to the simple "right after executor exit" approach).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21390
**[Test build #91128 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/91128/testReport)** for PR 21390 at commit [`2011eed`](https://github.com/apache/spark/commit/2011eede002664ef75e00f1f0228c5d765753f4c).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the issue:
https://github.com/apache/spark/pull/21390
Thanks @JoshRosen for very detailed and thoughtful reply.
Agreed TTL could be fragile, but I was very concern with what point
> There is a related issue where shuffle files can be leaked indefinitely following executor death because the external shuffle service is never directly told that shuffles are safe to remove (the context cleaner sends RPCs to executors and executors clean up their own shuffle files). That issue is substantially harder to fix, though, since it likely requires protocol changes to the shuffle service or an inversion-of-control where the shuffle service can periodically ask the driver "do any of these shuffle IDs correspond to cleaned shuffles?".
So will probably follow up with you at some point.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21390
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21390
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/91128/
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 #21390: [SPARK-24340][Core] Clean up non-shuffle disk blo...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/21390#discussion_r190705466
--- Diff: core/src/main/scala/org/apache/spark/deploy/worker/Worker.scala ---
@@ -97,6 +99,10 @@ private[deploy] class Worker(
private val APP_DATA_RETENTION_SECONDS =
conf.getLong("spark.worker.cleanup.appDataTtl", 7 * 24 * 3600)
+ // Whether or not cleanup the non-shuffle files on executor death.
+ private val CLEANUP_NON_SHUFFLE_FILES_ENABLED =
+ conf.getBoolean("spark.storage.cleanupFilesAfterExecutorDeath", true)
--- End diff --
@jerryshao, that's a good point; I prefer your suggested `*Exit` name.
If executors exit in a healthy / controlled fashion then we don't expect non-shuffle files to be leaked, but the cleanup logic still will be run.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21390
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 #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21390
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/3483/
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 #21390: [SPARK-24340][Core] Clean up non-shuffle disk blo...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/21390#discussion_r190105740
--- Diff: core/src/main/scala/org/apache/spark/deploy/worker/Worker.scala ---
@@ -97,6 +97,10 @@ private[deploy] class Worker(
private val APP_DATA_RETENTION_SECONDS =
conf.getLong("spark.worker.cleanup.appDataTtl", 7 * 24 * 3600)
+ // Whether or not cleanup the non-shuffle files on executor finishes.
+ private val CLEANUP_NON_SHUFFLE_FILES_ENABLED =
+ conf.getBoolean("spark.worker.cleanup.nonShuffleFiles.enabled", true)
--- End diff --
I think these configurations might actually be independent:
- `spark.worker.cleanup.enabled` controls whether directories from finished applications (i.e. terminated drivers) are cleaned up is a decision which can impact users' ability to debug failed applications. In environments where clusters / underlying physical workers are ephemeral this configuration tends to matter a little less, but in environments with long-lived machines which host many Spark drivers over a long period of time (e.g. on prem) this configuration can matter. For example, I think that setting `spark.worker.cleanup.enabled = true` could mean that completely applications' executor logs get purged from the underlying host machine after a time period.
- The new configuration being proposed here generally matters only _within a long-running Spark application_ and is kind of orthogonal to the choice to keep or discard failed applications' executor directories. I think that we're only removing files within block managers' nested folders, right? So there should be no user-controlled / user-generated files here, at least in theory. Therefore the impact of this configuration on ordinary users' ability to debug is likely to be small (I think you'd only ever really want access to the files that we're deleting if you're going to hex-dump files to debug corruption issues which trigger segfaults, an unlikely scenario).
Given the kind of different motivations and lifecycles for these cleanup mechanisms and the motivations for different choices of defaults, I'm thinking that it might make sense to rename the new configuration so that we don't confuse users via similar names.
What do you think about something like `spark.storage.removeNonShuffleBlockManagerFilesAfterExecutorDeath`? Really verbose, but a precise description of what's going on. I'm open to better name suggestions (since I tend to pick overly-complicated ones); this is just a conversation-starting strawman.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21390: [SPARK-24340][Core] Clean up non-shuffle disk block mana...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21390
**[Test build #91083 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/91083/testReport)** for PR 21390 at commit [`4a4ab59`](https://github.com/apache/spark/commit/4a4ab595a32537bd5ad022ec77f3e598a252a8ed).
* This patch **fails due to an unknown error code, -9**.
* 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