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 2021/07/13 18:12:13 UTC
[GitHub] [spark] otterc opened a new pull request #33329: [WIP][SPARK-35917][SHUFFLE][CORE]Disable push-based shuffle feature to prevent it from being used
otterc opened a new pull request #33329:
URL: https://github.com/apache/spark/pull/33329
This is WIP because #33118 (comment)
What changes were proposed in this pull request?
Push-based shuffle is partially merged in apache master but some of the tasks are still incomplete. Since 3.2 is going to cut soon, we will not be able to get the pending tasks reviewed and merged. Few of the pending tasks make protocol changes to the push-based shuffle protocols, so we would like to prevent users from enabling push-based shuffle both on the client and the server until push-based shuffle implementation is complete.
We can prevent push-based shuffle to be used by throwing UnsupportedOperationException both on the client and the server when the user tries to enable it.
Why are the changes needed?
The change is needed to prevent users from trying out push-based shuffle until it is complete.
Does this PR introduce any user-facing change?
Yes. It will prevent users to try out push-based shuffle until it is complete.
How was this patch tested?
It is a straightforward change that prevents users from enable push-based shuffle so haven't added a UT for 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] dongjoon-hyun commented on a change in pull request #33329: [WIP][SPARK-35917][SHUFFLE][CORE]Disable push-based shuffle feature to prevent it from being used
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #33329:
URL: https://github.com/apache/spark/pull/33329#discussion_r669151977
##########
File path: core/src/main/scala/org/apache/spark/internal/config/package.scala
##########
@@ -2079,7 +2079,7 @@ package object config {
"conjunction with the server side flag spark.shuffle.server.mergedShuffleFileManagerImpl " +
"which needs to be set with the appropriate " +
"org.apache.spark.network.shuffle.MergedShuffleFileManager implementation for push-based " +
- "shuffle to be enabled")
+ "shuffle to be enabled. Push-based shuffle is not yet supported.")
.version("3.1.0")
Review comment:
This configuration is added at 3.1.0 and do we want to add comments like `Push-based shuffle is not yet supported` at 3.1.3?
--
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] otterc commented on a change in pull request #33329: [WIP][SPARK-35917][SHUFFLE][CORE][3.2] Disable push-based shuffle feature to prevent it from being used
Posted by GitBox <gi...@apache.org>.
otterc commented on a change in pull request #33329:
URL: https://github.com/apache/spark/pull/33329#discussion_r669189297
##########
File path: core/src/main/scala/org/apache/spark/internal/config/package.scala
##########
@@ -2079,7 +2079,7 @@ package object config {
"conjunction with the server side flag spark.shuffle.server.mergedShuffleFileManagerImpl " +
"which needs to be set with the appropriate " +
"org.apache.spark.network.shuffle.MergedShuffleFileManager implementation for push-based " +
- "shuffle to be enabled")
+ "shuffle to be enabled. Push-based shuffle is not yet supported.")
.version("3.1.0")
Review comment:
In 3.1, multiple critical pieces of push-based shuffle were not there, for example SPARK-32922, SPARK-32921, and SPARK-32920. So, even if anyone even tried to turn it on, it would just not work.
However, in 3.2 we are just missing 2 correctness fixes which involve modifying push-based shuffle protocols: #33078 and #33034. Without these 2 push-based shuffle still works but this is what we want to prevent.
Given this I don't think it makes any difference if we add these comments to branches of version < 3.2. WDYT?
--
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] Ngone51 commented on pull request #33329: [WIP][SPARK-35917][SHUFFLE][CORE][3.2] Disable push-based shuffle feature to prevent it from being used
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on pull request #33329:
URL: https://github.com/apache/spark/pull/33329#issuecomment-880487790
I care more about the correctness issue here. So I feel it's safer to disallow use.
--
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 #33329: [WIP][SPARK-35917][SHUFFLE][CORE][3.2] Disable push-based shuffle feature to prevent it from being used
Posted by GitBox <gi...@apache.org>.
mridulm commented on pull request #33329:
URL: https://github.com/apache/spark/pull/33329#issuecomment-879652090
@HyukjinKwon, @dongjoon-hyun Fixing the pending jira's would require protocol changes - and shuffle protocol does not gracefully handle incompatible changes. More details [here](https://github.com/apache/spark/pull/33118#issuecomment-870084009) and [here](https://github.com/apache/spark/pull/33118#issuecomment-871135005).
I asked @otterc to keep this PR ready just in case we want to take the path of disabling push based shuffle entirely in 3.2.x line in case correctness fixes cant be resolved before GA - we should not be merging this unless we are sure the other two fixes wont make it through, and we want to disable it.
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] mridulm commented on pull request #33329: [WIP][SPARK-35917][SHUFFLE][CORE][3.2] Disable push-based shuffle feature to prevent it from being used
Posted by GitBox <gi...@apache.org>.
mridulm commented on pull request #33329:
URL: https://github.com/apache/spark/pull/33329#issuecomment-879652454
+CC @Ngone51
--
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] HyukjinKwon commented on pull request #33329: [WIP][SPARK-35917][SHUFFLE][CORE][3.2] Disable push-based shuffle feature to prevent it from being used
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #33329:
URL: https://github.com/apache/spark/pull/33329#issuecomment-879569468
Yeah, I think we won't necessarily have to make it failed when it's enabled. I believe it's fine to explicitly document that this feature is unstable, and either correctness or backward compatibility isn't guaranteed. In fact, I guess we haven't even properly documented this yet (?).
--
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] HyukjinKwon edited a comment on pull request #33329: [WIP][SPARK-35917][SHUFFLE][CORE][3.2] Disable push-based shuffle feature to prevent it from being used
Posted by GitBox <gi...@apache.org>.
HyukjinKwon edited a comment on pull request #33329:
URL: https://github.com/apache/spark/pull/33329#issuecomment-879569468
Yeah, I think we won't necessarily have to make it failed when it's enabled. I believe it's fine to explicitly document that this feature is unstable, and either correctness or backward compatibility isn't guaranteed at this moment - to be clear, is it not usable at all? In fact, I guess we haven't even properly documented this yet (?).
--
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 #33329: [WIP][SPARK-35917][SHUFFLE][CORE][3.2] Disable push-based shuffle feature to prevent it from being used
Posted by GitBox <gi...@apache.org>.
mridulm commented on pull request #33329:
URL: https://github.com/apache/spark/pull/33329#issuecomment-884313154
@otterc Can you please update to later master please ?
--
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] otterc commented on pull request #33329: [WIP][SPARK-35917][SHUFFLE][CORE]Disable push-based shuffle feature to prevent it from being used
Posted by GitBox <gi...@apache.org>.
otterc commented on pull request #33329:
URL: https://github.com/apache/spark/pull/33329#issuecomment-879299258
cc. @mridulm @Ngone51 @Victsm
--
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 #33329: [WIP][SPARK-35917][SHUFFLE][CORE][3.2] Disable push-based shuffle feature to prevent it from being used
Posted by GitBox <gi...@apache.org>.
mridulm commented on pull request #33329:
URL: https://github.com/apache/spark/pull/33329#issuecomment-923114031
Closing this PR as it is no longer relevant !
--
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 closed pull request #33329: [WIP][SPARK-35917][SHUFFLE][CORE][3.2] Disable push-based shuffle feature to prevent it from being used
Posted by GitBox <gi...@apache.org>.
mridulm closed pull request #33329:
URL: https://github.com/apache/spark/pull/33329
--
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] HyukjinKwon commented on pull request #33329: [WIP][SPARK-35917][SHUFFLE][CORE][3.2] Disable push-based shuffle feature to prevent it from being used
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #33329:
URL: https://github.com/apache/spark/pull/33329#issuecomment-879717569
I am fine either way. I was just thinking that it's fine to have some incompatibility or breaking stuff as long as we document so and it's disabled by default (since this push based shuffle is brand new, unstable and not really exposed to users yet in fact).
--
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] AmplabJenkins commented on pull request #33329: [WIP][SPARK-35917][SHUFFLE][CORE]Disable push-based shuffle feature to prevent it from being used
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33329:
URL: https://github.com/apache/spark/pull/33329#issuecomment-879306799
Can one of the admins verify this patch?
--
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 change in pull request #33329: [WIP][SPARK-35917][SHUFFLE][CORE]Disable push-based shuffle feature to prevent it from being used
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #33329:
URL: https://github.com/apache/spark/pull/33329#discussion_r669153013
##########
File path: core/src/main/scala/org/apache/spark/util/Utils.scala
##########
@@ -2598,11 +2598,16 @@ private[spark] object Utils extends Logging {
* Support push based shuffle with multiple app attempts
*/
def isPushBasedShuffleEnabled(conf: SparkConf): Boolean = {
- conf.get(PUSH_BASED_SHUFFLE_ENABLED) &&
+ val isPushBasedShuffleEnabled = conf.get(PUSH_BASED_SHUFFLE_ENABLED) &&
(conf.get(IS_TESTING).getOrElse(false) ||
(conf.get(SHUFFLE_SERVICE_ENABLED) &&
conf.get(SparkLauncher.SPARK_MASTER, null) == "yarn" &&
getYarnMaxAttempts(conf) == 1))
+ if (isPushBasedShuffleEnabled && !conf.get(IS_TESTING).getOrElse(false)) {
+ // TODO: Remove this once push-based shuffle is fully supported.
Review comment:
This `TODO` is not required because `branch-3.2` will never allow it if we didn't deliver at 3.2.0.
--
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] otterc commented on a change in pull request #33329: [WIP][SPARK-35917][SHUFFLE][CORE][3.2] Disable push-based shuffle feature to prevent it from being used
Posted by GitBox <gi...@apache.org>.
otterc commented on a change in pull request #33329:
URL: https://github.com/apache/spark/pull/33329#discussion_r669189297
##########
File path: core/src/main/scala/org/apache/spark/internal/config/package.scala
##########
@@ -2079,7 +2079,7 @@ package object config {
"conjunction with the server side flag spark.shuffle.server.mergedShuffleFileManagerImpl " +
"which needs to be set with the appropriate " +
"org.apache.spark.network.shuffle.MergedShuffleFileManager implementation for push-based " +
- "shuffle to be enabled")
+ "shuffle to be enabled. Push-based shuffle is not yet supported.")
.version("3.1.0")
Review comment:
In 3.1, multiple critical pieces of push-based shuffle were not there, for example SPARK-32922, SPARK-32921, and SPARK-32920. So, even if anyone even tried to turn it on, it would just not work.
However, in 3.2 we are just missing 2 correctness fixes which involve modifying push-based shuffle protocols: #33078 and #33034. Without these 2 push-based shuffle still works but this is what we want to prevent.
Given this I don't think it makes a difference if we add these comments to branches of version < 3.2. WDYT?
--
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 #33329: [WIP][SPARK-35917][SHUFFLE][CORE][3.2] Disable push-based shuffle feature to prevent it from being used
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #33329:
URL: https://github.com/apache/spark/pull/33329#issuecomment-879452177
I'm okay with disabling this, but I'm wondering what is the different from branch-3.1 because branch-3.1 also has the same configuration. If we want to disable this explicitly, it might be enough to comment on configuration doc. For the other Exceptions and assertions looks overkill. In any way, I'll leave this to the other committers.
--
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] HyukjinKwon commented on pull request #33329: [WIP][SPARK-35917][SHUFFLE][CORE][3.2] Disable push-based shuffle feature to prevent it from being used
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #33329:
URL: https://github.com/apache/spark/pull/33329#issuecomment-879717729
Being conservative sounds fine to me too since we're in core. I will leave it to you @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 edited a comment on pull request #33329: [WIP][SPARK-35917][SHUFFLE][CORE][3.2] Disable push-based shuffle feature to prevent it from being used
Posted by GitBox <gi...@apache.org>.
mridulm edited a comment on pull request #33329:
URL: https://github.com/apache/spark/pull/33329#issuecomment-884313154
@otterc Can you please update to later branch please ?
--
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 change in pull request #33329: [WIP][SPARK-35917][SHUFFLE][CORE]Disable push-based shuffle feature to prevent it from being used
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #33329:
URL: https://github.com/apache/spark/pull/33329#discussion_r669153013
##########
File path: core/src/main/scala/org/apache/spark/util/Utils.scala
##########
@@ -2598,11 +2598,16 @@ private[spark] object Utils extends Logging {
* Support push based shuffle with multiple app attempts
*/
def isPushBasedShuffleEnabled(conf: SparkConf): Boolean = {
- conf.get(PUSH_BASED_SHUFFLE_ENABLED) &&
+ val isPushBasedShuffleEnabled = conf.get(PUSH_BASED_SHUFFLE_ENABLED) &&
(conf.get(IS_TESTING).getOrElse(false) ||
(conf.get(SHUFFLE_SERVICE_ENABLED) &&
conf.get(SparkLauncher.SPARK_MASTER, null) == "yarn" &&
getYarnMaxAttempts(conf) == 1))
+ if (isPushBasedShuffleEnabled && !conf.get(IS_TESTING).getOrElse(false)) {
+ // TODO: Remove this once push-based shuffle is fully supported.
Review comment:
This `TODO` is not required because `branch-3.2` will never allow it in the future if we didn't deliver at 3.2.0.
--
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 edited a comment on pull request #33329: [WIP][SPARK-35917][SHUFFLE][CORE][3.2] Disable push-based shuffle feature to prevent it from being used
Posted by GitBox <gi...@apache.org>.
mridulm edited a comment on pull request #33329:
URL: https://github.com/apache/spark/pull/33329#issuecomment-879652090
@HyukjinKwon, @dongjoon-hyun Fixing the pending jira's would require protocol changes - and shuffle protocol does not gracefully handle incompatible changes. More details [here](https://github.com/apache/spark/pull/33118#issuecomment-870084009) and [here](https://github.com/apache/spark/pull/33118#issuecomment-871135005).
I asked @otterc to keep this PR ready (as WIP) just in case we want to take the path of disabling push based shuffle entirely in 3.2.x line in case correctness fixes cant be resolved before GA - we should not be merging this unless we are sure the other two fixes wont make it through, and we want to disable it.
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