You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "warrenzhu25 (via GitHub)" <gi...@apache.org> on 2023/07/09 04:49:58 UTC
[GitHub] [spark] warrenzhu25 opened a new pull request, #41906: [SPARK-44345][CORE] Only log unknown shuffle map output as error when shuffle migration disabled
warrenzhu25 opened a new pull request, #41906:
URL: https://github.com/apache/spark/pull/41906
### What changes were proposed in this pull request?
Only log unknown shuffle map output as error when shuffle migration disabled
### Why are the changes needed?
When decommission and shuffle migration is enabled, there're lots of error message like `Asked to update map output for unknown shuffle` .
As shuffle clean and unregister is done by `ContextCleaner` in an async way, when target block manager received the shuffle block from decommissioned block manager, then update shuffle location to map output tracker, but at that time, shuffle might have been unregistered. This should not consider as error.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Manually tested.
--
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 #41906: [SPARK-44345][CORE] Only log unknown shuffle map output as error when shuffle migration disabled
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #41906:
URL: https://github.com/apache/spark/pull/41906#discussion_r1278819867
##########
core/src/main/scala/org/apache/spark/MapOutputTracker.scala:
##########
@@ -805,8 +808,10 @@ private[spark] class MapOutputTrackerMaster(
shuffleStatuses.get(shuffleId) match {
case Some(shuffleStatus) =>
shuffleStatus.updateMapOutput(mapId, bmAddress)
+ case None if shuffleMigrationEnabled =>
+ logDebug(s"Asked to update map output for unknown shuffle ${shuffleId}")
case None =>
- logError(s"Asked to update map output for unknown shuffle ${shuffleId}")
+ logWarn(s"Asked to update map output for unknown shuffle ${shuffleId}")
Review Comment:
Let us not change this.
The discussion above was whether we should make something was was `error` earlier into `debug` or not.
Note - just because migration is enabled does not tell us if this failure is due to migration - it need not be.
--
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] warrenzhu25 commented on pull request #41906: [SPARK-44345][CORE] Only log unknown shuffle map output as error when shuffle migration disabled
Posted by "warrenzhu25 (via GitHub)" <gi...@apache.org>.
warrenzhu25 commented on PR #41906:
URL: https://github.com/apache/spark/pull/41906#issuecomment-1732381910
@dongjoon-hyun Could you help take a look?
--
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 #41906: [SPARK-44345][CORE] Lower `unknown shuffle map output` log level to WARN if shuffle migration is enabled
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #41906:
URL: https://github.com/apache/spark/pull/41906#issuecomment-1732456458
Merged to master for Apache Spark 4.0.0.
Thank. you, @warrenzhu25 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] warrenzhu25 commented on pull request #41906: [SPARK-44345][CORE] Only log unknown shuffle map output as error when shuffle migration disabled
Posted by "warrenzhu25 (via GitHub)" <gi...@apache.org>.
warrenzhu25 commented on PR #41906:
URL: https://github.com/apache/spark/pull/41906#issuecomment-1627604202
cc @holdenk @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] warrenzhu25 commented on pull request #41906: [SPARK-44345][CORE] Only log unknown shuffle map output as warn when shuffle migration enabled
Posted by "warrenzhu25 (via GitHub)" <gi...@apache.org>.
warrenzhu25 commented on PR #41906:
URL: https://github.com/apache/spark/pull/41906#issuecomment-1732455235
> > @dongjoon-hyun Could you help take a look?
>
> Hi, @warrenzhu25 . I don't think [#41906 (comment)](https://github.com/apache/spark/pull/41906#discussion_r1278819867) is addressed properly. This PR is still not ready due to that. IIUC, he asked not to change it explicitly. In addition, that change is not consistent with the AS-IS PR title neither.
Updated.
--
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] warrenzhu25 commented on a diff in pull request #41906: [SPARK-44345][CORE] Only log unknown shuffle map output as error when shuffle migration disabled
Posted by "warrenzhu25 (via GitHub)" <gi...@apache.org>.
warrenzhu25 commented on code in PR #41906:
URL: https://github.com/apache/spark/pull/41906#discussion_r1258852062
##########
core/src/main/scala/org/apache/spark/MapOutputTracker.scala:
##########
@@ -805,6 +808,8 @@ private[spark] class MapOutputTrackerMaster(
shuffleStatuses.get(shuffleId) match {
case Some(shuffleStatus) =>
shuffleStatus.updateMapOutput(mapId, bmAddress)
+ case None if shuffleMigrationEnabled =>
+ logDebug(s"Asked to update map output for unknown shuffle ${shuffleId}")
Review Comment:
How about making it `info` or `warn` level?
--
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 #41906: [SPARK-44345][CORE] Only log unknown shuffle map output as error when shuffle migration disabled
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #41906:
URL: https://github.com/apache/spark/pull/41906#discussion_r1264346284
##########
core/src/main/scala/org/apache/spark/MapOutputTracker.scala:
##########
@@ -805,6 +808,8 @@ private[spark] class MapOutputTrackerMaster(
shuffleStatuses.get(shuffleId) match {
case Some(shuffleStatus) =>
shuffleStatus.updateMapOutput(mapId, bmAddress)
+ case None if shuffleMigrationEnabled =>
+ logDebug(s"Asked to update map output for unknown shuffle ${shuffleId}")
Review Comment:
I am fine with warn.
+CC @dongjoon-hyun as well - since you should be debugging this issue a lot more than me :-)
--
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 #41906: [SPARK-44345][CORE] Only log unknown shuffle map output as error when shuffle migration disabled
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #41906:
URL: https://github.com/apache/spark/pull/41906#issuecomment-1732430297
> @dongjoon-hyun Could you help take a look?
Hi, @warrenzhu25 . I don't think https://github.com/apache/spark/pull/41906#discussion_r1278819867 is addressed properly. This PR is still not ready due to that. IIUC, he asked not to change it explicitly. In addition, that change is not consistent with the AS-IS PR title neither.
--
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 #41906: [SPARK-44345][CORE] Lower `unknown shuffle map output` log level to WARN if shuffle migration is enabled
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #41906: [SPARK-44345][CORE] Lower `unknown shuffle map output` log level to WARN if shuffle migration is enabled
URL: https://github.com/apache/spark/pull/41906
--
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 #41906: [SPARK-44345][CORE] Only log unknown shuffle map output as error when shuffle migration disabled
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #41906:
URL: https://github.com/apache/spark/pull/41906#discussion_r1257430884
##########
core/src/main/scala/org/apache/spark/MapOutputTracker.scala:
##########
@@ -805,6 +808,8 @@ private[spark] class MapOutputTrackerMaster(
shuffleStatuses.get(shuffleId) match {
case Some(shuffleStatus) =>
shuffleStatus.updateMapOutput(mapId, bmAddress)
+ case None if shuffleMigrationEnabled =>
+ logDebug(s"Asked to update map output for unknown shuffle ${shuffleId}")
Review Comment:
I am very hesitant with making this debug.
If missing, in non migration context, it expected to be logged as an error - making it debug would essentially suppress that message.
--
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 #41906: [SPARK-44345][CORE] Only log unknown shuffle map output as error when shuffle migration disabled
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #41906:
URL: https://github.com/apache/spark/pull/41906#discussion_r1278819867
##########
core/src/main/scala/org/apache/spark/MapOutputTracker.scala:
##########
@@ -805,8 +808,10 @@ private[spark] class MapOutputTrackerMaster(
shuffleStatuses.get(shuffleId) match {
case Some(shuffleStatus) =>
shuffleStatus.updateMapOutput(mapId, bmAddress)
+ case None if shuffleMigrationEnabled =>
+ logDebug(s"Asked to update map output for unknown shuffle ${shuffleId}")
case None =>
- logError(s"Asked to update map output for unknown shuffle ${shuffleId}")
+ logWarn(s"Asked to update map output for unknown shuffle ${shuffleId}")
Review Comment:
Let us not change this.
The discussion above was whether we should make something which was `error` earlier into `debug` or not.
Note - just because migration is enabled does not tell us if this failure is due to migration - it need not be.
--
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