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/09/12 02:58:57 UTC
[GitHub] [spark] LuciferYang opened a new pull request, #37853: [SPARK-40404][DOCS] Fix the error description related to `spark.shuffle.service.db` in the document
LuciferYang opened a new pull request, #37853:
URL: https://github.com/apache/spark/pull/37853
### What changes were proposed in this pull request?
From the context from [pr](https://github.com/apache/spark/pull/19032) of [SPARK-17321](https://issues.apache.org/jira/browse/SPARK-17321), `YarnShuffleService` will persist data into `Level/RocksDB` when Yarn NM recovery is enabled. This behavior is not controlled by `spark.shuffle.service.db.enabled` and is not always enabled.
So the description of `spark.shuffle.service.db.enabled` in `spark-standalone.md` is misleading.
This pr deletes the part related to yarn in the description of `spark.shuffle.service.db.enabled`, and adds the precondition description related to `Yarn NodeManager recovery is enabled` for `spark.shuffle.service.db.backend`.
### Why are the changes needed?
Fix document description.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Pass GitHub Actions
--
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 #37853: [SPARK-40404][DOCS] Fix the wrong description related to `spark.shuffle.service.db.enabled` in the document
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37853:
URL: https://github.com/apache/spark/pull/37853#discussion_r968137075
##########
docs/spark-standalone.md:
##########
@@ -322,9 +322,9 @@ SPARK_WORKER_OPTS supports the following system properties:
<td>true</td>
<td>
Store External Shuffle service state on local disk so that when the external shuffle service is restarted, it will
- automatically reload info on current executors. This only affects standalone mode (yarn always has this behavior
- enabled). You should also enable <code>spark.worker.cleanup.enabled</code>, to ensure that the state
- eventually gets cleaned up. This config may be removed in the future.
+ automatically reload info on current executors. This only affects standalone mode. You should also enable
+ <code>spark.worker.cleanup.enabled</code>, to ensure that the state eventually gets cleaned up.
+ This config may be removed in the future.
Review Comment:
Or can we change
```
yarn always has this behavior enabled
```
to
```
The behavior of yarn (and mesos) not related to this configuration
```
?
--
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 #37853: [SPARK-40404][DOCS] Fix the wrong description related to `spark.shuffle.service.db.enabled` in the document
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37853:
URL: https://github.com/apache/spark/pull/37853#discussion_r968124459
##########
docs/spark-standalone.md:
##########
@@ -322,9 +322,9 @@ SPARK_WORKER_OPTS supports the following system properties:
<td>true</td>
<td>
Store External Shuffle service state on local disk so that when the external shuffle service is restarted, it will
- automatically reload info on current executors. This only affects standalone mode (yarn always has this behavior
- enabled). You should also enable <code>spark.worker.cleanup.enabled</code>, to ensure that the state
- eventually gets cleaned up. This config may be removed in the future.
+ automatically reload info on current executors. This only affects standalone mode. You should also enable
+ <code>spark.worker.cleanup.enabled</code>, to ensure that the state eventually gets cleaned up.
+ This config may be removed in the future.
Review Comment:
- when `yarn.nodemanager.recovery.enabled` is true, `_recoveryPath` and `registeredExecutorFile` in `YarnShuffleService` will not null, then `YarnShuffleService` persist data into Level/RocksDB
- when `yarn.nodemanager.recovery.enabled` is false, `_recoveryPath` and `registeredExecutorFile` in `YarnShuffleService` will null, then `YarnShuffleService` not persist data into diskstore
The persist behavior of YarnShuffleService is controlled by Yarn's configuration. It seems that it not related to `spark.shuffle.service.db.enabled`, so I don't think it is necessary to mention `yarn always has this behavior enabled` in this configuration description.
If we need add yarn related descriptions here, should we also need mention `mesos always has this behavior disabled` here...
--
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 #37853: [SPARK-40404][DOCS] Fix the wrong description related to `spark.shuffle.service.db.enabled` in the document
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37853:
URL: https://github.com/apache/spark/pull/37853#discussion_r968137075
##########
docs/spark-standalone.md:
##########
@@ -322,9 +322,9 @@ SPARK_WORKER_OPTS supports the following system properties:
<td>true</td>
<td>
Store External Shuffle service state on local disk so that when the external shuffle service is restarted, it will
- automatically reload info on current executors. This only affects standalone mode (yarn always has this behavior
- enabled). You should also enable <code>spark.worker.cleanup.enabled</code>, to ensure that the state
- eventually gets cleaned up. This config may be removed in the future.
+ automatically reload info on current executors. This only affects standalone mode. You should also enable
+ <code>spark.worker.cleanup.enabled</code>, to ensure that the state eventually gets cleaned up.
+ This config may be removed in the future.
Review Comment:
Or can we change
```
yarn always has this behavior enabled
```
to
```
The behavior of yarn (and mesos) not depend on this configuration
```
?
--
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 #37853: [SPARK-40404][DOCS] Fix the wrong description related to `spark.shuffle.service.db.enabled` in the document
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37853:
URL: https://github.com/apache/spark/pull/37853#discussion_r968087123
##########
docs/spark-standalone.md:
##########
@@ -322,9 +322,9 @@ SPARK_WORKER_OPTS supports the following system properties:
<td>true</td>
<td>
Store External Shuffle service state on local disk so that when the external shuffle service is restarted, it will
- automatically reload info on current executors. This only affects standalone mode (yarn always has this behavior
- enabled). You should also enable <code>spark.worker.cleanup.enabled</code>, to ensure that the state
- eventually gets cleaned up. This config may be removed in the future.
+ automatically reload info on current executors. This only affects standalone mode. You should also enable
+ <code>spark.worker.cleanup.enabled</code>, to ensure that the state eventually gets cleaned up.
+ This config may be removed in the future.
Review Comment:
Hmm... does `yarn always has this behavior enabled` mean that YarnShuffleService will always will persist data into Level/RocksDB?
Is that incorrect?
--
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] dongjoon-hyun closed pull request #37853: [SPARK-40404][DOCS] Add precondition description for `spark.shuffle.service.db.backend` in `running-on-yarn.md`
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun closed pull request #37853: [SPARK-40404][DOCS] Add precondition description for `spark.shuffle.service.db.backend` in `running-on-yarn.md`
URL: https://github.com/apache/spark/pull/37853
--
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 #37853: [SPARK-40404][DOCS] Add precondition description for `spark.shuffle.service.db.backend` in `running-on-yarn.md`
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #37853:
URL: https://github.com/apache/spark/pull/37853#discussion_r971042246
##########
docs/running-on-yarn.md:
##########
@@ -852,8 +852,8 @@ The following extra configuration options are available when the shuffle service
<td><code>spark.shuffle.service.db.backend</code></td>
<td>LEVELDB</td>
<td>
- To specify the kind of disk-base store used in shuffle service state store, supports `LEVELDB` and `ROCKSDB` now
- and `LEVELDB` as default value.
+ When work-preserving restart is enabled in YARN, this is used to specify the disk-base store used
+ in shuffle service state store, supports `LEVELDB` and `ROCKSDB` with `LEVELDB` as default value.
The original data store in `LevelDB/RocksDB` will not be automatically convert to another kind of storage now.
Review Comment:
`convert` -> `converted` ?
--
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 #37853: [SPARK-40404][DOCS] Add precondition description for `spark.shuffle.service.db.backend` in `running-on-yarn.md`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37853:
URL: https://github.com/apache/spark/pull/37853#discussion_r971589945
##########
docs/running-on-yarn.md:
##########
@@ -852,8 +852,8 @@ The following extra configuration options are available when the shuffle service
<td><code>spark.shuffle.service.db.backend</code></td>
<td>LEVELDB</td>
<td>
- To specify the kind of disk-base store used in shuffle service state store, supports `LEVELDB` and `ROCKSDB` now
- and `LEVELDB` as default value.
+ When work-preserving restart is enabled in YARN, this is used to specify the disk-base store used
+ in shuffle service state store, supports `LEVELDB` and `ROCKSDB` with `LEVELDB` as default value.
The original data store in `LevelDB/RocksDB` will not be automatically convert to another kind of storage now.
Review Comment:
The old one will not be deleted, but the new one will be created. When the store type is switched, the directory name will change, for example, from `registeredExecutors.ldb` to `registeredExecutors.rdb`, `YarnShuffleService` will create `registeredExecutors.rdb` if it not exists, but `YarnShuffleService` did not know that `registeredExecutors.ldb` existed, so it will not be deleted
--
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 #37853: [SPARK-40404][DOCS] Fix the wrong description related to `spark.shuffle.service.db.enabled` in the document
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #37853:
URL: https://github.com/apache/spark/pull/37853#discussion_r968078477
##########
docs/spark-standalone.md:
##########
@@ -322,9 +322,9 @@ SPARK_WORKER_OPTS supports the following system properties:
<td>true</td>
<td>
Store External Shuffle service state on local disk so that when the external shuffle service is restarted, it will
- automatically reload info on current executors. This only affects standalone mode (yarn always has this behavior
- enabled). You should also enable <code>spark.worker.cleanup.enabled</code>, to ensure that the state
- eventually gets cleaned up. This config may be removed in the future.
+ automatically reload info on current executors. This only affects standalone mode. You should also enable
+ <code>spark.worker.cleanup.enabled</code>, to ensure that the state eventually gets cleaned up.
+ This config may be removed in the future.
Review Comment:
Why are we removing the yarn related blurb from here ? Essentially, this boolean does not control the behavior in yarn - for yarn, that is configured for the cluster, and inherits the behavior for spark
--
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 #37853: [SPARK-40404][DOCS] Fix the wrong description related to `spark.shuffle.service.db.enabled` in the document
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37853:
URL: https://github.com/apache/spark/pull/37853#discussion_r968094267
##########
docs/running-on-yarn.md:
##########
@@ -852,8 +852,8 @@ The following extra configuration options are available when the shuffle service
<td><code>spark.shuffle.service.db.backend</code></td>
<td>LEVELDB</td>
<td>
- To specify the kind of disk-base store used in shuffle service state store, supports `LEVELDB` and `ROCKSDB` now
- and `LEVELDB` as default value.
+ When Yarn NodeManager recovery is enabled, this use to specify the kind of disk-base store used in shuffle
+ service state store, supports `LEVELDB` and `ROCKSDB` now and `LEVELDB` as default value.
Review Comment:
[d8b39ef](https://github.com/apache/spark/pull/37853/commits/d8b39eff34d8e3cf2382caa377355b6e41ec1eb4) fix this
--
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 #37853: [SPARK-40404][DOCS] Fix the wrong description related to `spark.shuffle.service.db.enabled` in the document
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37853:
URL: https://github.com/apache/spark/pull/37853#discussion_r970309147
##########
docs/spark-standalone.md:
##########
@@ -322,9 +322,9 @@ SPARK_WORKER_OPTS supports the following system properties:
<td>true</td>
<td>
Store External Shuffle service state on local disk so that when the external shuffle service is restarted, it will
- automatically reload info on current executors. This only affects standalone mode (yarn always has this behavior
- enabled). You should also enable <code>spark.worker.cleanup.enabled</code>, to ensure that the state
- eventually gets cleaned up. This config may be removed in the future.
+ automatically reload info on current executors. This only affects standalone mode. You should also enable
+ <code>spark.worker.cleanup.enabled</code>, to ensure that the state eventually gets cleaned up.
+ This config may be removed in the future.
Review Comment:
Have an offline discussion with the author @weixiuli of this configuration description, [b92cf5c](https://github.com/apache/spark/pull/37853/commits/b92cf5c1e2e4a17a2bad052ed6b0f40cb0dbcb2d) revert change in 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] LuciferYang commented on a diff in pull request #37853: [SPARK-40404][DOCS] Add precondition description for `spark.shuffle.service.db.backend` in `running-on-yarn.md`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37853:
URL: https://github.com/apache/spark/pull/37853#discussion_r972569300
##########
docs/running-on-yarn.md:
##########
@@ -852,8 +852,8 @@ The following extra configuration options are available when the shuffle service
<td><code>spark.shuffle.service.db.backend</code></td>
<td>LEVELDB</td>
<td>
- To specify the kind of disk-base store used in shuffle service state store, supports `LEVELDB` and `ROCKSDB` now
- and `LEVELDB` as default value.
+ When work-preserving restart is enabled in YARN, this is used to specify the disk-base store used
+ in shuffle service state store, supports `LEVELDB` and `ROCKSDB` with `LEVELDB` as default value.
The original data store in `LevelDB/RocksDB` will not be automatically convert to another kind of storage now.
Review Comment:
@dongjoon-hyun @mridulm Automatic data format conversion may be a useful feature. I think it is more friendly for migrating stock users to use new features. I have filed a Jira SPARK-40464 and will promote its completion if necessary.
--
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 #37853: [SPARK-40404][DOCS] Fix the wrong description related to `spark.shuffle.service.db.enabled` in the document
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37853:
URL: https://github.com/apache/spark/pull/37853#discussion_r969111813
##########
docs/spark-standalone.md:
##########
@@ -322,9 +322,9 @@ SPARK_WORKER_OPTS supports the following system properties:
<td>true</td>
<td>
Store External Shuffle service state on local disk so that when the external shuffle service is restarted, it will
- automatically reload info on current executors. This only affects standalone mode (yarn always has this behavior
- enabled). You should also enable <code>spark.worker.cleanup.enabled</code>, to ensure that the state
- eventually gets cleaned up. This config may be removed in the future.
+ automatically reload info on current executors. This only affects standalone mode. You should also enable
+ <code>spark.worker.cleanup.enabled</code>, to ensure that the state eventually gets cleaned up.
+ This config may be removed in the future.
Review Comment:
friendly ping @weixiuli @squito to help check this change
--
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 #37853: [SPARK-40404][DOCS] Fix the wrong description related to `spark.shuffle.service.db.enabled` in the document
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37853:
URL: https://github.com/apache/spark/pull/37853#discussion_r968124459
##########
docs/spark-standalone.md:
##########
@@ -322,9 +322,9 @@ SPARK_WORKER_OPTS supports the following system properties:
<td>true</td>
<td>
Store External Shuffle service state on local disk so that when the external shuffle service is restarted, it will
- automatically reload info on current executors. This only affects standalone mode (yarn always has this behavior
- enabled). You should also enable <code>spark.worker.cleanup.enabled</code>, to ensure that the state
- eventually gets cleaned up. This config may be removed in the future.
+ automatically reload info on current executors. This only affects standalone mode. You should also enable
+ <code>spark.worker.cleanup.enabled</code>, to ensure that the state eventually gets cleaned up.
+ This config may be removed in the future.
Review Comment:
- when `yarn.nodemanager.recovery.enabled` is true, `_recoveryPath` and `registeredExecutorFile` in `YarnShuffleService` will not null, then `YarnShuffleService` persist data into Level/RocksDB
- when `yarn.nodemanager.recovery.enabled` is false, `_recoveryPath` and `registeredExecutorFile` in `YarnShuffleService` will null, then `YarnShuffleService` not persist data into diskstore
YarnShuffleService's behavior is controlled by Yarn's configuration. It seems that it not related to `spark.shuffle.service.db.enabled`, so I don't think it is necessary to mention `yarn always has this behavior enabled` in this configuration description.
If we need add yarn related descriptions here, should we also need mention `mesos always has this behavior disabled` here...
--
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 #37853: [SPARK-40404][DOCS] Fix the wrong description related to `spark.shuffle.service.db.enabled` in the document
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37853:
URL: https://github.com/apache/spark/pull/37853#discussion_r968127667
##########
docs/running-on-yarn.md:
##########
@@ -852,8 +852,8 @@ The following extra configuration options are available when the shuffle service
<td><code>spark.shuffle.service.db.backend</code></td>
<td>LEVELDB</td>
<td>
- To specify the kind of disk-base store used in shuffle service state store, supports `LEVELDB` and `ROCKSDB` now
- and `LEVELDB` as default value.
+ When Yarn NodeManager recovery is enabled, this use to specify the kind of disk-base store used in shuffle
+ service state store, supports `LEVELDB` and `ROCKSDB` now and `LEVELDB` as default value.
Review Comment:
Thanks for your correction
--
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 #37853: [SPARK-40404][DOCS] Fix the wrong description related to `spark.shuffle.service.db.enabled` in the document
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37853:
URL: https://github.com/apache/spark/pull/37853#discussion_r968087123
##########
docs/spark-standalone.md:
##########
@@ -322,9 +322,9 @@ SPARK_WORKER_OPTS supports the following system properties:
<td>true</td>
<td>
Store External Shuffle service state on local disk so that when the external shuffle service is restarted, it will
- automatically reload info on current executors. This only affects standalone mode (yarn always has this behavior
- enabled). You should also enable <code>spark.worker.cleanup.enabled</code>, to ensure that the state
- eventually gets cleaned up. This config may be removed in the future.
+ automatically reload info on current executors. This only affects standalone mode. You should also enable
+ <code>spark.worker.cleanup.enabled</code>, to ensure that the state eventually gets cleaned up.
+ This config may be removed in the future.
Review Comment:
Hmm... does `yarn always has this behavior enabled` mean that YarnShuffleService will always persist data into Level/RocksDB?
Is that incorrect?
--
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] dongjoon-hyun commented on a diff in pull request #37853: [SPARK-40404][DOCS] Add precondition description for `spark.shuffle.service.db.backend` in `running-on-yarn.md`
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #37853:
URL: https://github.com/apache/spark/pull/37853#discussion_r971564123
##########
docs/running-on-yarn.md:
##########
@@ -852,8 +852,8 @@ The following extra configuration options are available when the shuffle service
<td><code>spark.shuffle.service.db.backend</code></td>
<td>LEVELDB</td>
<td>
- To specify the kind of disk-base store used in shuffle service state store, supports `LEVELDB` and `ROCKSDB` now
- and `LEVELDB` as default value.
+ When work-preserving restart is enabled in YARN, this is used to specify the disk-base store used
+ in shuffle service state store, supports `LEVELDB` and `ROCKSDB` with `LEVELDB` as default value.
The original data store in `LevelDB/RocksDB` will not be automatically convert to another kind of storage now.
Review Comment:
+1 for @mridulm comment. And, could you add additional description about what happens at the runtime when the the store types are mismatched. It's deleted and recreated, right?
--
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 #37853: [SPARK-40404][DOCS] Add precondition description for `spark.shuffle.service.db.backend` in `running-on-yarn.md`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37853:
URL: https://github.com/apache/spark/pull/37853#discussion_r971600317
##########
docs/running-on-yarn.md:
##########
@@ -852,8 +852,8 @@ The following extra configuration options are available when the shuffle service
<td><code>spark.shuffle.service.db.backend</code></td>
<td>LEVELDB</td>
<td>
- To specify the kind of disk-base store used in shuffle service state store, supports `LEVELDB` and `ROCKSDB` now
- and `LEVELDB` as default value.
+ When work-preserving restart is enabled in YARN, this is used to specify the disk-base store used
+ in shuffle service state store, supports `LEVELDB` and `ROCKSDB` with `LEVELDB` as default value.
The original data store in `LevelDB/RocksDB` will not be automatically convert to another kind of storage now.
Review Comment:
Add `The original data store will be retained and the new type data store will be created when switching storage types.` Is that ok ?
--
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 #37853: [SPARK-40404][DOCS] Fix the wrong description related to `spark.shuffle.service.db.enabled` in the document
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #37853:
URL: https://github.com/apache/spark/pull/37853#discussion_r968077550
##########
docs/running-on-yarn.md:
##########
@@ -852,8 +852,8 @@ The following extra configuration options are available when the shuffle service
<td><code>spark.shuffle.service.db.backend</code></td>
<td>LEVELDB</td>
<td>
- To specify the kind of disk-base store used in shuffle service state store, supports `LEVELDB` and `ROCKSDB` now
- and `LEVELDB` as default value.
+ When Yarn NodeManager recovery is enabled, this use to specify the kind of disk-base store used in shuffle
+ service state store, supports `LEVELDB` and `ROCKSDB` now and `LEVELDB` as default value.
Review Comment:
```suggestion
When work-preserving restart is enabled in YARN, this is used to specify the disk-base store used in shuffle
service state store, supports `LEVELDB` and `ROCKSDB` with `LEVELDB` as default value.
```
--
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 #37853: [SPARK-40404][DOCS] Add precondition description for `spark.shuffle.service.db.backend` in `running-on-yarn.md`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37853:
URL: https://github.com/apache/spark/pull/37853#discussion_r972570934
##########
docs/running-on-yarn.md:
##########
@@ -852,8 +852,8 @@ The following extra configuration options are available when the shuffle service
<td><code>spark.shuffle.service.db.backend</code></td>
<td>LEVELDB</td>
<td>
- To specify the kind of disk-base store used in shuffle service state store, supports `LEVELDB` and `ROCKSDB` now
- and `LEVELDB` as default value.
+ When work-preserving restart is enabled in YARN, this is used to specify the disk-base store used
+ in shuffle service state store, supports `LEVELDB` and `ROCKSDB` with `LEVELDB` as default value.
The original data store in `LevelDB/RocksDB` will not be automatically convert to another kind of storage now.
Review Comment:
> @dongjoon-hyun @mridulm Automatic data format conversion may be a useful feature. I think it is more friendly for migrating stock users to use new features. I have filed a Jira [SPARK-40464](https://issues.apache.org/jira/browse/SPARK-40464) and will promote its completion if necessary.
also cc @panbingkun , what I discussed with you offline yesterday
--
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 #37853: [SPARK-40404][DOCS] Add precondition description for `spark.shuffle.service.db.backend` in `running-on-yarn.md`
Posted by GitBox <gi...@apache.org>.
mridulm commented on PR #37853:
URL: https://github.com/apache/spark/pull/37853#issuecomment-1250357195
+CC @dongjoon-hyun for review
--
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] dongjoon-hyun commented on pull request #37853: [SPARK-40404][DOCS] Add precondition description for `spark.shuffle.service.db.backend` in `running-on-yarn.md`
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #37853:
URL: https://github.com/apache/spark/pull/37853#issuecomment-1250479587
Merged to master for Apache Spark 3.4. Thank you, @LuciferYang and @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 #37853: [SPARK-40404][DOCS] Fix the wrong description related to `spark.shuffle.service.db.enabled` in the document
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37853:
URL: https://github.com/apache/spark/pull/37853#discussion_r968087123
##########
docs/spark-standalone.md:
##########
@@ -322,9 +322,9 @@ SPARK_WORKER_OPTS supports the following system properties:
<td>true</td>
<td>
Store External Shuffle service state on local disk so that when the external shuffle service is restarted, it will
- automatically reload info on current executors. This only affects standalone mode (yarn always has this behavior
- enabled). You should also enable <code>spark.worker.cleanup.enabled</code>, to ensure that the state
- eventually gets cleaned up. This config may be removed in the future.
+ automatically reload info on current executors. This only affects standalone mode. You should also enable
+ <code>spark.worker.cleanup.enabled</code>, to ensure that the state eventually gets cleaned up.
+ This config may be removed in the future.
Review Comment:
Hmm... does `yarn always has this behavior enabled` mean that YarnShuffleService will always will persist data into Level/RocksDB?
Isn't that right?
--
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 #37853: [SPARK-40404][DOCS] Fix the wrong description related to `spark.shuffle.service.db.enabled` in the document
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #37853:
URL: https://github.com/apache/spark/pull/37853#discussion_r968078477
##########
docs/spark-standalone.md:
##########
@@ -322,9 +322,9 @@ SPARK_WORKER_OPTS supports the following system properties:
<td>true</td>
<td>
Store External Shuffle service state on local disk so that when the external shuffle service is restarted, it will
- automatically reload info on current executors. This only affects standalone mode (yarn always has this behavior
- enabled). You should also enable <code>spark.worker.cleanup.enabled</code>, to ensure that the state
- eventually gets cleaned up. This config may be removed in the future.
+ automatically reload info on current executors. This only affects standalone mode. You should also enable
+ <code>spark.worker.cleanup.enabled</code>, to ensure that the state eventually gets cleaned up.
+ This config may be removed in the future.
Review Comment:
Why are we removing the yarn related blurb from here ?
--
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 #37853: [SPARK-40404][DOCS] Fix the wrong description related to `spark.shuffle.service.db.enabled` in the document
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #37853:
URL: https://github.com/apache/spark/pull/37853#discussion_r968124459
##########
docs/spark-standalone.md:
##########
@@ -322,9 +322,9 @@ SPARK_WORKER_OPTS supports the following system properties:
<td>true</td>
<td>
Store External Shuffle service state on local disk so that when the external shuffle service is restarted, it will
- automatically reload info on current executors. This only affects standalone mode (yarn always has this behavior
- enabled). You should also enable <code>spark.worker.cleanup.enabled</code>, to ensure that the state
- eventually gets cleaned up. This config may be removed in the future.
+ automatically reload info on current executors. This only affects standalone mode. You should also enable
+ <code>spark.worker.cleanup.enabled</code>, to ensure that the state eventually gets cleaned up.
+ This config may be removed in the future.
Review Comment:
- when `yarn.nodemanager.recovery.enabled` is true, `_recoveryPath` and `registeredExecutorFile` in `YarnShuffleService` will not null, then `YarnShuffleService` persist data into Level/RocksDB
- when `yarn.nodemanager.recovery.enabled` is false, `_recoveryPath` and `registeredExecutorFile` in `YarnShuffleService` will null, then `YarnShuffleService` not persist data into diskstore
YarnShuffleService'` behavior is controlled by Yarn's configuration. It seems that it not related to `spark.shuffle.service.db.enabled`, so I don't think it is necessary to mention `yarn always has this behavior enabled` in this configuration description.
If we need add yarn related descriptions here, should we also need mention `mesos always has this behavior disabled` here...
--
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