You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/12/27 03:04:35 UTC
[GitHub] [spark] LuciferYang opened a new pull request, #39226: [SPARK-41694][CORE] Add new config to clean up `spark.ui.store.path` directory when `SparkContext.stop()`
LuciferYang opened a new pull request, #39226:
URL: https://github.com/apache/spark/pull/39226
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] mridulm commented on a diff in pull request #39226: [SPARK-41694][CORE] Add new config to clean up `spark.ui.store.path` directory when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #39226:
URL: https://github.com/apache/spark/pull/39226#discussion_r1059289062
##########
core/src/main/scala/org/apache/spark/status/AppStatusStore.scala:
##########
@@ -733,6 +734,15 @@ private[spark] class AppStatusStore(
def close(): Unit = {
store.close()
+ cleanUpStorePath()
+ }
+
+ private def cleanUpStorePath(): Unit = {
+ storePath.foreach { p =>
+ if (p.exists()) {
+ p.listFiles().foreach(Utils.deleteRecursively)
+ }
+ }
Review Comment:
I was specifically referring to the context of driver using the kvstore for the live UI data.
The codepath you [referred to](https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/status/KVUtils.scala#L155) is for use in history server (particularly after a restart/upgrade of history server) - not relevant for driver.
It becomes relevant to driver when multiple applications end up using the same path - primarily in client mode.
Once we have support for persisting the kvstore for history server consumption, this could change - but until then, I would say this should be an internal impl detail, and get managed/deleted by driver - just like how we manage other temp files today.
Thoughts ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #39226: [SPARK-41694][CORE] Isolate RocksDB path for Live UI and automatically cleanup when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #39226:
URL: https://github.com/apache/spark/pull/39226#issuecomment-1371675917
@gengliangwang has been re-triggered the failed task, should not related to this pr ~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gengliangwang commented on pull request #39226: [SPARK-41694][CORE] Isolate RocksDB path for Live UI and automatically cleanup when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on PR #39226:
URL: https://github.com/apache/spark/pull/39226#issuecomment-1371644321
@LuciferYang The failed test doesn't seem related. Just to double confirm, could you retrigger it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #39226: [SPARK-41694][CORE] Add new config to clean up `spark.ui.store.path` directory when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #39226:
URL: https://github.com/apache/spark/pull/39226#discussion_r1059225398
##########
core/src/main/scala/org/apache/spark/status/AppStatusStore.scala:
##########
@@ -733,6 +734,15 @@ private[spark] class AppStatusStore(
def close(): Unit = {
store.close()
+ cleanUpStorePath()
+ }
+
+ private def cleanUpStorePath(): Unit = {
+ storePath.foreach { p =>
+ if (p.exists()) {
+ p.listFiles().foreach(Utils.deleteRecursively)
+ }
+ }
Review Comment:
I think we can let the user specify a basedir and create a subdirectory under the basedir to ensure that rocksdb is isolated with app granularity. At the same time, this subdirectory is automatically created when Spark starts and automatically deleted when it stops.
I want to change this pr according to this ideas, and then we can see if it is acceptable @gengliangwang @mridulm
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #39226: [SPARK-41694][CORE] Add new config to clean up `spark.ui.store.path` directory when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #39226:
URL: https://github.com/apache/spark/pull/39226#discussion_r1059332142
##########
core/src/main/scala/org/apache/spark/status/AppStatusStore.scala:
##########
@@ -733,6 +734,15 @@ private[spark] class AppStatusStore(
def close(): Unit = {
store.close()
+ cleanUpStorePath()
+ }
+
+ private def cleanUpStorePath(): Unit = {
+ storePath.foreach { p =>
+ if (p.exists()) {
+ p.listFiles().foreach(Utils.deleteRecursively)
+ }
+ }
Review Comment:
Let me think more
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gengliangwang commented on a diff in pull request #39226: [SPARK-41694][CORE] Add new config to clean up `spark.ui.store.path` directory when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on code in PR #39226:
URL: https://github.com/apache/spark/pull/39226#discussion_r1059204173
##########
core/src/main/scala/org/apache/spark/status/AppStatusStore.scala:
##########
@@ -733,6 +734,15 @@ private[spark] class AppStatusStore(
def close(): Unit = {
store.close()
+ cleanUpStorePath()
+ }
+
+ private def cleanUpStorePath(): Unit = {
+ storePath.foreach { p =>
+ if (p.exists()) {
+ p.listFiles().foreach(Utils.deleteRecursively)
+ }
+ }
Review Comment:
> I would argue always clean up - we dont need config for it. Just as we cleanup all other temp paths created during spark lifecycle.
OK let's simply cleanup them without a configuration now. My initial concern is that some users may want to leverage the files in SHS. Since we don't have a design for it, we can just remove them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #39226: [SPARK-41694][CORE] Add new config to clean up `spark.ui.store.path` directory when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #39226:
URL: https://github.com/apache/spark/pull/39226#discussion_r1058030818
##########
core/src/main/scala/org/apache/spark/status/AppStatusStore.scala:
##########
@@ -733,6 +734,15 @@ private[spark] class AppStatusStore(
def close(): Unit = {
store.close()
+ cleanUpStorePath()
+ }
+
+ private def cleanUpStorePath(): Unit = {
+ storePath.foreach { p =>
+ if (p.exists()) {
+ p.listFiles().foreach(Utils.deleteRecursively)
+ }
+ }
Review Comment:
Yes, abnormal termination will generate dangling files, users need to manually cleanup in this scenario.
There's another problem I need to mention:
- Start spark-shell twice in the same directory with `spark.ui.store.path` configured:
- The first app start successfully and use `spark.ui.store.path` to store LiveUI data
- The second app will also start successfully, but `InMemoryStore` will be used to store Live UI data due to `org.rocksdb.RocksDBException: While lock file: /${baseDir}/listing.rdb/LOCK: Resource temporarily unavailable`
Should we solve this problem, such as creating a subdirectory under `spark.ui.store.path` for each different App to ensure the isolation of rocksdb usage? (However, if we do this, do we still need to add the `cleanup` config? The app will use different directories after restart) @gengliangwang @mridulm
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] mridulm commented on a diff in pull request #39226: [SPARK-41694][CORE] Add new config to clean up `spark.ui.store.path` directory when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #39226:
URL: https://github.com/apache/spark/pull/39226#discussion_r1057840697
##########
core/src/main/scala/org/apache/spark/status/AppStatusStore.scala:
##########
@@ -733,6 +734,15 @@ private[spark] class AppStatusStore(
def close(): Unit = {
store.close()
+ cleanUpStorePath()
+ }
+
+ private def cleanUpStorePath(): Unit = {
+ storePath.foreach { p =>
+ if (p.exists()) {
+ p.listFiles().foreach(Utils.deleteRecursively)
+ }
+ }
Review Comment:
Anything under the application directory will be cleaned up, right ?
Are we expecting this to be set outside of it ? (If yes, we will have dangling files any abnormal termination).
Btw, why not simply `storePath.foreach(Utils.deleteRecursively)
##########
core/src/main/scala/org/apache/spark/status/AppStatusStore.scala:
##########
@@ -733,6 +734,15 @@ private[spark] class AppStatusStore(
def close(): Unit = {
store.close()
+ cleanUpStorePath()
+ }
+
+ private def cleanUpStorePath(): Unit = {
+ storePath.foreach { p =>
+ if (p.exists()) {
+ p.listFiles().foreach(Utils.deleteRecursively)
+ }
+ }
Review Comment:
Anything under the application directory will be cleaned up, right ?
Are we expecting this to be set outside of it ? (If yes, we will have dangling files any abnormal termination).
Btw, why not simply `storePath.foreach(Utils.deleteRecursively)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gengliangwang commented on a diff in pull request #39226: [SPARK-41694][CORE] Isolate RocksDB path for Live UI and automatically cleanup when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on code in PR #39226:
URL: https://github.com/apache/spark/pull/39226#discussion_r1061892643
##########
core/src/test/scala/org/apache/spark/status/AutoCleanupLiveUIDirSuite.scala:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.status
+
+import org.apache.spark.{SparkConf, SparkContext, SparkFunSuite}
+import org.apache.spark.internal.config.Status.LIVE_UI_LOCAL_STORE_DIR
+import org.apache.spark.network.util.JavaUtils
+import org.apache.spark.util.Utils
+
+class AutoCleanupLiveUIDirSuite extends SparkFunSuite {
+
+ test(s"auto cleanup spark ui store path") {
Review Comment:
```suggestion
test("SPARK-41694: Auto cleanup Spark UI store path") {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #39226: [SPARK-41694][CORE] Add new config to clean up `spark.ui.store.path` directory when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #39226:
URL: https://github.com/apache/spark/pull/39226#discussion_r1057419363
##########
core/src/main/scala/org/apache/spark/status/AppStatusStore.scala:
##########
@@ -36,7 +36,8 @@ import org.apache.spark.util.kvstore.KVStore
*/
private[spark] class AppStatusStore(
val store: KVStore,
- val listener: Option[AppStatusListener] = None) {
+ val listener: Option[AppStatusListener] = None,
+ val storePath: Option[File] = None) {
Review Comment:
A new parameter has been added, which can also be changed into a class field with init value `None`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #39226: [SPARK-41694][CORE] Add new config to clean up `spark.ui.store.path` directory when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #39226:
URL: https://github.com/apache/spark/pull/39226#discussion_r1059266699
##########
core/src/main/scala/org/apache/spark/status/AppStatusStore.scala:
##########
@@ -733,6 +734,15 @@ private[spark] class AppStatusStore(
def close(): Unit = {
store.close()
+ cleanUpStorePath()
+ }
+
+ private def cleanUpStorePath(): Unit = {
+ storePath.foreach { p =>
+ if (p.exists()) {
+ p.listFiles().foreach(Utils.deleteRecursively)
+ }
+ }
Review Comment:
A little busy today, will update later
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] mridulm commented on a diff in pull request #39226: [SPARK-41694][CORE] Add new config to clean up `spark.ui.store.path` directory when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #39226:
URL: https://github.com/apache/spark/pull/39226#discussion_r1059289062
##########
core/src/main/scala/org/apache/spark/status/AppStatusStore.scala:
##########
@@ -733,6 +734,15 @@ private[spark] class AppStatusStore(
def close(): Unit = {
store.close()
+ cleanUpStorePath()
+ }
+
+ private def cleanUpStorePath(): Unit = {
+ storePath.foreach { p =>
+ if (p.exists()) {
+ p.listFiles().foreach(Utils.deleteRecursively)
+ }
+ }
Review Comment:
I was specifically referring to the context of driver using the kvstore for the live UI data.
The codepath you [referred to](https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/status/KVUtils.scala#L155) is for use in history server (particularly after a restart/upgrade of history server) - not relevant for driver.
It becomes relevant to driver when multiple applications end up using the same path - primarily in client mode.
Once we have support for persisting the kvstore for history server consumption, this could change - but until then, I would say this should be an internal impl detail, and get managed/deleted by driver - just like how we manage other temp files today.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gengliangwang closed pull request #39226: [SPARK-41694][CORE] Isolate RocksDB path for Live UI and automatically cleanup when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
gengliangwang closed pull request #39226: [SPARK-41694][CORE] Isolate RocksDB path for Live UI and automatically cleanup when `SparkContext.stop()`
URL: https://github.com/apache/spark/pull/39226
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #39226: [SPARK-41694][CORE] Add new config to clean up `spark.ui.store.path` directory when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #39226:
URL: https://github.com/apache/spark/pull/39226#discussion_r1058754762
##########
core/src/main/scala/org/apache/spark/status/AppStatusStore.scala:
##########
@@ -733,6 +734,15 @@ private[spark] class AppStatusStore(
def close(): Unit = {
store.close()
+ cleanUpStorePath()
+ }
+
+ private def cleanUpStorePath(): Unit = {
+ storePath.foreach { p =>
+ if (p.exists()) {
+ p.listFiles().foreach(Utils.deleteRecursively)
+ }
+ }
Review Comment:
Got it, I think this is a good suggestion and agree with you, let's wait for @gengliangwang
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #39226: [SPARK-41694][CORE] Add new config to clean up `spark.ui.store.path` directory when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #39226:
URL: https://github.com/apache/spark/pull/39226#discussion_r1058024767
##########
core/src/main/scala/org/apache/spark/status/AppStatusStore.scala:
##########
@@ -733,6 +734,15 @@ private[spark] class AppStatusStore(
def close(): Unit = {
store.close()
+ cleanUpStorePath()
+ }
+
+ private def cleanUpStorePath(): Unit = {
+ storePath.foreach { p =>
+ if (p.exists()) {
+ p.listFiles().foreach(Utils.deleteRecursively)
+ }
+ }
Review Comment:
[c1b5447](https://github.com/apache/spark/pull/39226/commits/c1b54476bbad9f768588d23e12083a7880fec5f0) change to use `storePath.foreach(Utils.deleteRecursively)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #39226: [SPARK-41694][CORE] Isolate RocksDB path for Live UI and automatically cleanup when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #39226:
URL: https://github.com/apache/spark/pull/39226#issuecomment-1371772823
Thanks @gengliangwang
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #39226: [SPARK-41694][CORE] Add new config to clean up `spark.ui.store.path` directory when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #39226:
URL: https://github.com/apache/spark/pull/39226#discussion_r1060249072
##########
core/src/main/scala/org/apache/spark/status/AppStatusStore.scala:
##########
@@ -733,6 +734,15 @@ private[spark] class AppStatusStore(
def close(): Unit = {
store.close()
+ cleanUpStorePath()
+ }
+
+ private def cleanUpStorePath(): Unit = {
+ storePath.foreach { p =>
+ if (p.exists()) {
+ p.listFiles().foreach(Utils.deleteRecursively)
+ }
+ }
Review Comment:
The current code uses a strategy similar to `DiskBlockManager` to generate a temporary directory for RocksDB (without exposing `spark.ui.store.path` to users).
However, I think it is reasonable to expose `spark.ui.store.path` to users: when multiple long running Spark Apps(client mode) are running on same machine and RockDB is used to store the Live UI, the default use of `/tmp` to store data may cause `no space`, because `/tmp` directory is generally limited (about 5~7G space)
WDYT @gengliangwang @mridulm ? If `spark.ui.store.path` needs to be exposed to users, I need to revert the last commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] mridulm commented on a diff in pull request #39226: [SPARK-41694][CORE] Add new config to clean up `spark.ui.store.path` directory when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #39226:
URL: https://github.com/apache/spark/pull/39226#discussion_r1059289062
##########
core/src/main/scala/org/apache/spark/status/AppStatusStore.scala:
##########
@@ -733,6 +734,15 @@ private[spark] class AppStatusStore(
def close(): Unit = {
store.close()
+ cleanUpStorePath()
+ }
+
+ private def cleanUpStorePath(): Unit = {
+ storePath.foreach { p =>
+ if (p.exists()) {
+ p.listFiles().foreach(Utils.deleteRecursively)
+ }
+ }
Review Comment:
I was specifically referring to the context of driver using the kvstore for the live UI data.
The codepath you [referred to](https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/status/KVUtils.scala#L155) is for use in history server (particularly after a restart/upgrade of history server) - not relevant for driver.
It becomes relevant to driver when multiple applications end up using the same path - primarily in client mode.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gengliangwang commented on pull request #39226: [SPARK-41694][CORE] Isolate RocksDB path for Live UI and automatically cleanup when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on PR #39226:
URL: https://github.com/apache/spark/pull/39226#issuecomment-1370579527
@LuciferYang Thanks for the updates. I am back to work today. I wonder if we can have one rockdb instance for all the applications..so that SHS can leverage the rocksdb files more easily. I am take a closer look tomorrow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #39226: [SPARK-41694][CORE] Add new config to clean up `spark.ui.store.path` directory when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #39226:
URL: https://github.com/apache/spark/pull/39226#discussion_r1059225398
##########
core/src/main/scala/org/apache/spark/status/AppStatusStore.scala:
##########
@@ -733,6 +734,15 @@ private[spark] class AppStatusStore(
def close(): Unit = {
store.close()
+ cleanUpStorePath()
+ }
+
+ private def cleanUpStorePath(): Unit = {
+ storePath.foreach { p =>
+ if (p.exists()) {
+ p.listFiles().foreach(Utils.deleteRecursively)
+ }
+ }
Review Comment:
I think we can let the user specify a basedir and create a subdirectory under the basedir to ensure that rocksdb is isolated with `App` granularity. At the same time, this subdirectory is automatically created when Spark starts and automatically deleted when it stops.
I want to change this pr according to this ideas, and then we can see if it is acceptable @gengliangwang @mridulm
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] mridulm commented on a diff in pull request #39226: [SPARK-41694][CORE] Add new config to clean up `spark.ui.store.path` directory when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #39226:
URL: https://github.com/apache/spark/pull/39226#discussion_r1058705480
##########
core/src/main/scala/org/apache/spark/status/AppStatusStore.scala:
##########
@@ -733,6 +734,15 @@ private[spark] class AppStatusStore(
def close(): Unit = {
store.close()
+ cleanUpStorePath()
+ }
+
+ private def cleanUpStorePath(): Unit = {
+ storePath.foreach { p =>
+ if (p.exists()) {
+ p.listFiles().foreach(Utils.deleteRecursively)
+ }
+ }
Review Comment:
There are three separate cases here, though related, let us look at them individually.
a) Should we cleanup ?
For cluster mode, it is not very useful - since the cluster manager will do the same.
In client mode, like the excellent `spark-shell` example you gave @LuciferYang, this is indeed very relevant.
Given this, let us keep both modes consistent and cleanup for both (like what we do for block manager dirs for example).
b) Should we need a config to clean up live ui local files.
I would argue always clean up - we dont need config for it. Just as we cleanup all other temp paths created during spark lifecycle.
Is there any case where we do need to control this ? +CC @gengliangwang
c) Issue with rocks db locking.
We should treat this as a bug - couple of ways to fix it, though I would say we can simply follow what we already have in spark - for example, use `getLocalDir`.
Is there a reason to expose the ability to configure this path ? Or should it simply be an implementation detail, and something spark automatically creates during start and remove at stop ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gengliangwang commented on pull request #39226: [SPARK-41694][CORE] Isolate RocksDB path for Live UI and automatically cleanup when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on PR #39226:
URL: https://github.com/apache/spark/pull/39226#issuecomment-1371770833
Thanks, merging to master
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gengliangwang commented on a diff in pull request #39226: [SPARK-41694][CORE] Add new config to clean up `spark.ui.store.path` directory when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on code in PR #39226:
URL: https://github.com/apache/spark/pull/39226#discussion_r1057947130
##########
core/src/main/scala/org/apache/spark/internal/config/Status.scala:
##########
@@ -77,4 +77,12 @@ private[spark] object Status {
.version("3.4.0")
.stringConf
.createOptional
+
+ val LIVE_UI_LOCAL_STORE_DIR_AUTO_CLEANUP_ENABLED =
+ ConfigBuilder("spark.ui.store.path.auto.cleanup.enabled")
Review Comment:
The new conf name is a bit long. How about `spark.ui.store.clean.enabled`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #39226: [SPARK-41694][CORE] Add new config to clean up `spark.ui.store.path` directory when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #39226:
URL: https://github.com/apache/spark/pull/39226#discussion_r1058030818
##########
core/src/main/scala/org/apache/spark/status/AppStatusStore.scala:
##########
@@ -733,6 +734,15 @@ private[spark] class AppStatusStore(
def close(): Unit = {
store.close()
+ cleanUpStorePath()
+ }
+
+ private def cleanUpStorePath(): Unit = {
+ storePath.foreach { p =>
+ if (p.exists()) {
+ p.listFiles().foreach(Utils.deleteRecursively)
+ }
+ }
Review Comment:
Yes, abnormal termination will generate dangling files, users need to manually cleanup in this scenario.
There's another problem I need to mention:
- Start spark-shell twice in the same directory with `spark.ui.store.path` configured:
- The first app start successfully and use `spark.ui.store.path` to store LiveUI data
- The second app will also start successfully, but `InMemoryStore` will be used to store Live UI data due to
`org.rocksdb.RocksDBException: While lock file: /${baseDir}/listing.rdb/LOCK: Resource temporarily unavailable`
Should we solve this problem, such as creating a subdirectory under `spark.ui.store.path` for each different App to ensure the isolation of rocksdb usage? (However, if we do this, do we still need to add the `cleanup` config? The app will use different directories after restart) @gengliangwang @mridulm
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #39226: [SPARK-41694][CORE] Add new config to clean up `spark.ui.store.path` directory when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #39226:
URL: https://github.com/apache/spark/pull/39226#discussion_r1058030818
##########
core/src/main/scala/org/apache/spark/status/AppStatusStore.scala:
##########
@@ -733,6 +734,15 @@ private[spark] class AppStatusStore(
def close(): Unit = {
store.close()
+ cleanUpStorePath()
+ }
+
+ private def cleanUpStorePath(): Unit = {
+ storePath.foreach { p =>
+ if (p.exists()) {
+ p.listFiles().foreach(Utils.deleteRecursively)
+ }
+ }
Review Comment:
Yes, abnormal termination will generate dangling files, users need to manually cleanup in this scenario.
There's another problem I need to mention:
- Start spark-shell twice in the same directory with `spark.ui.store.path` configured:
- The first app start successfully and use `spark.ui.store.path` to store LiveUI data
- The second app will also start successfully, but `InMemoryStore` will be used to store Live UI data due to `org.rocksdb.RocksDBException: While lock file: /${baseDir}/listing.rdb/LOCK: Resource temporarily unavailable`
Should we solve this problem, such as creating a subdirectory under `spark.ui.store.path` for each different App to ensure the isolation of rocksdb usage? (However, if we do this, do we still need to add the `cleanup` config? The app will use different directories after restart)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gengliangwang commented on a diff in pull request #39226: [SPARK-41694][CORE] Add new config to clean up `spark.ui.store.path` directory when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on code in PR #39226:
URL: https://github.com/apache/spark/pull/39226#discussion_r1059204601
##########
core/src/main/scala/org/apache/spark/status/AppStatusStore.scala:
##########
@@ -733,6 +734,15 @@ private[spark] class AppStatusStore(
def close(): Unit = {
store.close()
+ cleanUpStorePath()
+ }
+
+ private def cleanUpStorePath(): Unit = {
+ storePath.foreach { p =>
+ if (p.exists()) {
+ p.listFiles().foreach(Utils.deleteRecursively)
+ }
+ }
Review Comment:
> Is there a reason to expose the ability to configure this path ?
Do you mean the RocksDB file path? The size can be big if a user increases the retention limits. Thus a user should be able to choose the file path for it, instead of automatically choosing a temp dir.
If there is a corrupted RocksDB file during startup, shall we just remove them and try creating the RocksDB file again? There is such code path: https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/status/KVUtils.scala#L155
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] mridulm commented on pull request #39226: [SPARK-41694][CORE] Isolate RocksDB path for Live UI and automatically cleanup when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
mridulm commented on PR #39226:
URL: https://github.com/apache/spark/pull/39226#issuecomment-1372604586
Late LGTM, thanks for working on this @LuciferYang !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #39226: [SPARK-41694][CORE] Add new config to clean up `spark.ui.store.path` directory when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #39226:
URL: https://github.com/apache/spark/pull/39226#discussion_r1059224030
##########
core/src/main/scala/org/apache/spark/status/AppStatusStore.scala:
##########
@@ -733,6 +734,15 @@ private[spark] class AppStatusStore(
def close(): Unit = {
store.close()
+ cleanUpStorePath()
+ }
+
+ private def cleanUpStorePath(): Unit = {
+ storePath.foreach { p =>
+ if (p.exists()) {
+ p.listFiles().foreach(Utils.deleteRecursively)
+ }
+ }
Review Comment:
> There's another problem I need to mention:
>
> * Start spark-shell twice in the same directory with `spark.ui.store.path` configured:
>
> * The first app start successfully and use `spark.ui.store.path` to store LiveUI data
> * The second app will also start successfully, but `InMemoryStore` will be used to store Live UI data due to
>
> `org.rocksdb.RocksDBException: While lock file: /${baseDir}/listing.rdb/LOCK: Resource temporarily unavailable`
@gengliangwang This scenario is a special case, we should not delete it, otherwise, the first app will be affected(in the scenario, the first app not stop)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #39226: [SPARK-41694][CORE] Add new config to clean up `spark.ui.store.path` directory when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #39226:
URL: https://github.com/apache/spark/pull/39226#issuecomment-1365582194
cc @gengliangwang
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gengliangwang commented on a diff in pull request #39226: [SPARK-41694][CORE] Add new config to clean up `spark.ui.store.path` directory when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on code in PR #39226:
URL: https://github.com/apache/spark/pull/39226#discussion_r1057948247
##########
docs/configuration.md:
##########
@@ -1388,6 +1388,14 @@ Apart from these, the following properties are also available, and may be useful
</td>
<td>3.4.0</td>
</tr>
+<tr>
+ <td><code>spark.ui.store.path.auto.cleanup.enabled</code></td>
+ <td>true</td>
+ <td>
+ If true, automatically cleanup the the data of `spark.ui.store.path` when SparkContext stop.
Review Comment:
stop => stops
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] mridulm commented on a diff in pull request #39226: [SPARK-41694][CORE] Add new config to clean up `spark.ui.store.path` directory when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #39226:
URL: https://github.com/apache/spark/pull/39226#discussion_r1057840697
##########
core/src/main/scala/org/apache/spark/status/AppStatusStore.scala:
##########
@@ -733,6 +734,15 @@ private[spark] class AppStatusStore(
def close(): Unit = {
store.close()
+ cleanUpStorePath()
+ }
+
+ private def cleanUpStorePath(): Unit = {
+ storePath.foreach { p =>
+ if (p.exists()) {
+ p.listFiles().foreach(Utils.deleteRecursively)
+ }
+ }
Review Comment:
Anything under the application directory will be cleaned up, right ?
Are we expecting this to be set outside of it ? (If yes, we will have dangling files for any abnormal termination).
Btw, why not simply `storePath.foreach(Utils.deleteRecursively)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #39226: [SPARK-41694][CORE] Isolate RocksDB path for Live UI and automatically cleanup when `SparkContext.stop()`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #39226:
URL: https://github.com/apache/spark/pull/39226#issuecomment-1370585016
As far as I know, it should not be possible for multiple processes to write one RocksDB instance at the same time. Of course, it may can be implemented in a way I do not know
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org