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/10/24 07:43:27 UTC
[GitHub] [spark] JiexingLi opened a new pull request, #38371: Fix some comments in DAGSchedulerSuite
JiexingLi opened a new pull request, #38371:
URL: https://github.com/apache/spark/pull/38371
### What changes were proposed in this pull request?
Fix some wrong or confusing comments in DAGSchedulerSuite.
--
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 #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #38371:
URL: https://github.com/apache/spark/pull/38371#discussion_r1009018882
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3089,13 +3089,14 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
submit(finalRdd, Array(0, 1), properties = new Properties())
// Finish the first 2 shuffle map stages.
- completeShuffleMapStageSuccessfully(0, 0, 2)
+ completeShuffleMapStageSuccessfully(0, 0, 2, Seq("hostA", "hostB"))
assert(mapOutputTracker.findMissingPartitions(shuffleId1) === Some(Seq.empty))
completeShuffleMapStageSuccessfully(1, 0, 2, Seq("hostB", "hostD"))
assert(mapOutputTracker.findMissingPartitions(shuffleId2) === Some(Seq.empty))
- // Executor lost on hostB, both of stage 0 and 1 should be reran.
+ // FetchFailed on stage 2, both of stage 1 and 2 should be reran. Besides, executor lost on
+ // hostB, both of stage 0 and 1 should be reran.
Review Comment:
The comment could read - "Executor lost on hostB, both of stage 0 and 1 should be rerun - as part of recomputation of stage 2" : since there is output on hostB for stage 0 (see completeShuffleMapStageSuccessfully), and stage 1.
But looks fine to me even without the 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] mridulm commented on pull request #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
mridulm commented on PR #38371:
URL: https://github.com/apache/spark/pull/38371#issuecomment-1299617022
Merged to master, thanks or fixing this @JiexingLi !
Thanks for looking into this @HyukjinKwon :-)
--
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 #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #38371:
URL: https://github.com/apache/spark/pull/38371#discussion_r1009018499
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3089,13 +3089,14 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
submit(finalRdd, Array(0, 1), properties = new Properties())
// Finish the first 2 shuffle map stages.
- completeShuffleMapStageSuccessfully(0, 0, 2)
+ completeShuffleMapStageSuccessfully(0, 0, 2, Seq("hostA", "hostB"))
Review Comment:
This change is not required.
Fetch failed is due to stage 1 partition on hostB going missing - by default, `completeShuffleMapStageSuccessfully` will progressively complete on hostA, hostB, etc ... - it will result in recomputing 0 (since there are two partitions - on hostA and hostB) and 1 (and 2 ofcourse).
In this case, since there is output on hostB for stage 0, it is recomputed.
--
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] JiexingLi commented on a diff in pull request #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
JiexingLi commented on code in PR #38371:
URL: https://github.com/apache/spark/pull/38371#discussion_r1010030101
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3089,13 +3089,14 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
submit(finalRdd, Array(0, 1), properties = new Properties())
// Finish the first 2 shuffle map stages.
- completeShuffleMapStageSuccessfully(0, 0, 2)
+ completeShuffleMapStageSuccessfully(0, 0, 2, Seq("hostA", "hostB"))
assert(mapOutputTracker.findMissingPartitions(shuffleId1) === Some(Seq.empty))
completeShuffleMapStageSuccessfully(1, 0, 2, Seq("hostB", "hostD"))
assert(mapOutputTracker.findMissingPartitions(shuffleId2) === Some(Seq.empty))
- // Executor lost on hostB, both of stage 0 and 1 should be reran.
+ // FetchFailed on stage 2, both of stage 1 and 2 should be reran. Besides, executor lost on
+ // hostB, both of stage 0 and 1 should be reran.
Review Comment:
Thanks, Mridul.
I updated the comment. Your rewrite is very neat and clear.
--
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 #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #38371:
URL: https://github.com/apache/spark/pull/38371#discussion_r1009018882
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3089,13 +3089,14 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
submit(finalRdd, Array(0, 1), properties = new Properties())
// Finish the first 2 shuffle map stages.
- completeShuffleMapStageSuccessfully(0, 0, 2)
+ completeShuffleMapStageSuccessfully(0, 0, 2, Seq("hostA", "hostB"))
assert(mapOutputTracker.findMissingPartitions(shuffleId1) === Some(Seq.empty))
completeShuffleMapStageSuccessfully(1, 0, 2, Seq("hostB", "hostD"))
assert(mapOutputTracker.findMissingPartitions(shuffleId2) === Some(Seq.empty))
- // Executor lost on hostB, both of stage 0 and 1 should be reran.
+ // FetchFailed on stage 2, both of stage 1 and 2 should be reran. Besides, executor lost on
+ // hostB, both of stage 0 and 1 should be reran.
Review Comment:
The comment could read - "Executor lost on hostB, both of stage 0 (if missing partitions) and 1 should be reran - as part of recomputation of stage 2" : if no output on hostB for stage 0, we can omit that from the list of recomputed stages.
Will that clarify ?
--
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 #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #38371:
URL: https://github.com/apache/spark/pull/38371#discussion_r1009018882
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3089,13 +3089,14 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
submit(finalRdd, Array(0, 1), properties = new Properties())
// Finish the first 2 shuffle map stages.
- completeShuffleMapStageSuccessfully(0, 0, 2)
+ completeShuffleMapStageSuccessfully(0, 0, 2, Seq("hostA", "hostB"))
assert(mapOutputTracker.findMissingPartitions(shuffleId1) === Some(Seq.empty))
completeShuffleMapStageSuccessfully(1, 0, 2, Seq("hostB", "hostD"))
assert(mapOutputTracker.findMissingPartitions(shuffleId2) === Some(Seq.empty))
- // Executor lost on hostB, both of stage 0 and 1 should be reran.
+ // FetchFailed on stage 2, both of stage 1 and 2 should be reran. Besides, executor lost on
+ // hostB, both of stage 0 and 1 should be reran.
Review Comment:
The comment could read - "Executor lost on hostB, both of stage 0 and 1 should be rerun - as part of recomputation of stage 2" : since there is output on hostB for stage 0 (in completeShuffleMapStageSuccessfully), and stage 1.
But looks fine to me even without the 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] mridulm commented on a diff in pull request #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #38371:
URL: https://github.com/apache/spark/pull/38371#discussion_r1009018499
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3089,13 +3089,14 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
submit(finalRdd, Array(0, 1), properties = new Properties())
// Finish the first 2 shuffle map stages.
- completeShuffleMapStageSuccessfully(0, 0, 2)
+ completeShuffleMapStageSuccessfully(0, 0, 2, Seq("hostA", "hostB"))
Review Comment:
This change is not required.
Fetch failed is due to stage 1 partition on hostB going missing - it does not matter where stage 0 ran - it will result in recomputing 0 and 1 (and 2 ofcourse)
--
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 #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #38371:
URL: https://github.com/apache/spark/pull/38371#discussion_r1009018882
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3089,13 +3089,14 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
submit(finalRdd, Array(0, 1), properties = new Properties())
// Finish the first 2 shuffle map stages.
- completeShuffleMapStageSuccessfully(0, 0, 2)
+ completeShuffleMapStageSuccessfully(0, 0, 2, Seq("hostA", "hostB"))
assert(mapOutputTracker.findMissingPartitions(shuffleId1) === Some(Seq.empty))
completeShuffleMapStageSuccessfully(1, 0, 2, Seq("hostB", "hostD"))
assert(mapOutputTracker.findMissingPartitions(shuffleId2) === Some(Seq.empty))
- // Executor lost on hostB, both of stage 0 and 1 should be reran.
+ // FetchFailed on stage 2, both of stage 1 and 2 should be reran. Besides, executor lost on
+ // hostB, both of stage 0 and 1 should be reran.
Review Comment:
The comment could read - "Executor lost on hostB, both of stage 0 and 1 should be reran - as part of recomputation of stage 2" : but looks fine to me as-is.
--
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] JiexingLi commented on a diff in pull request #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
JiexingLi commented on code in PR #38371:
URL: https://github.com/apache/spark/pull/38371#discussion_r1009037092
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3089,13 +3089,14 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
submit(finalRdd, Array(0, 1), properties = new Properties())
// Finish the first 2 shuffle map stages.
- completeShuffleMapStageSuccessfully(0, 0, 2)
+ completeShuffleMapStageSuccessfully(0, 0, 2, Seq("hostA", "hostB"))
Review Comment:
Yes, this is not required. It is only added for better readability (as I mentioned, "hostA", "hostB" are the default hosts). In my opinion, having the value here, we don't need to go to read completeShuffleMapStageSuccessfully(), but only the code here, we can know what happen. Beside, it might be good to keep the two completeShuffleMapStageSuccessfully() consistent here (both having hostNames, or both not having hostNames)? Let me know if you think I should delete Seq("hostA", "hostB") here.
I added "In case no hostNames are provided, the tasks will progressively complete on hostA, hostB, etc." to completeShuffleMapStageSuccessfully().
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3207,7 +3209,7 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
assert(failure == null, "job should not fail")
val failedStages = scheduler.failedStages.toSeq
assert(failedStages.length == 2)
- // Shuffle blocks of "hostA" is lost, so first task of the `shuffleMapRdd2` needs to retry.
+ // Shuffle blocks of "hostA" is lost, so first task of the `finalRdd` needs to retry.
Review Comment:
Yes, my bad.
Thanks and 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] HyukjinKwon commented on a diff in pull request #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on code in PR #38371:
URL: https://github.com/apache/spark/pull/38371#discussion_r1008974030
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3089,13 +3089,14 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
submit(finalRdd, Array(0, 1), properties = new Properties())
// Finish the first 2 shuffle map stages.
- completeShuffleMapStageSuccessfully(0, 0, 2)
+ completeShuffleMapStageSuccessfully(0, 0, 2, Seq("hostA", "hostB"))
Review Comment:
This not only fixes the comment but the test codes. Mind elaborating this 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] JiexingLi commented on pull request #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
JiexingLi commented on PR #38371:
URL: https://github.com/apache/spark/pull/38371#issuecomment-1296430263
Thank @HyukjinKwon and @itholic for the information. I have now updated the PR description and added the ticket for the title.
Can you help take another look? Thank you so much.
--
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] JiexingLi commented on a diff in pull request #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
JiexingLi commented on code in PR #38371:
URL: https://github.com/apache/spark/pull/38371#discussion_r1010029712
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3089,13 +3089,14 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
submit(finalRdd, Array(0, 1), properties = new Properties())
// Finish the first 2 shuffle map stages.
- completeShuffleMapStageSuccessfully(0, 0, 2)
+ completeShuffleMapStageSuccessfully(0, 0, 2, Seq("hostA", "hostB"))
Review Comment:
Sounds good. Deleted the hostNames param.
--
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 #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #38371:
URL: https://github.com/apache/spark/pull/38371#discussion_r1009018882
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3089,13 +3089,14 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
submit(finalRdd, Array(0, 1), properties = new Properties())
// Finish the first 2 shuffle map stages.
- completeShuffleMapStageSuccessfully(0, 0, 2)
+ completeShuffleMapStageSuccessfully(0, 0, 2, Seq("hostA", "hostB"))
assert(mapOutputTracker.findMissingPartitions(shuffleId1) === Some(Seq.empty))
completeShuffleMapStageSuccessfully(1, 0, 2, Seq("hostB", "hostD"))
assert(mapOutputTracker.findMissingPartitions(shuffleId2) === Some(Seq.empty))
- // Executor lost on hostB, both of stage 0 and 1 should be reran.
+ // FetchFailed on stage 2, both of stage 1 and 2 should be reran. Besides, executor lost on
+ // hostB, both of stage 0 and 1 should be reran.
Review Comment:
The comment could read - "Executor lost on hostB, both of stage 0 (if missing partitions) and 1 should be reran - as part of recomputation of stage 2" : if no output on hostB for stage 0 (which appears to be the case), we can omit that from the list of recomputed stages in the comment.
Will that clarify ?
--
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 #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #38371:
URL: https://github.com/apache/spark/pull/38371#discussion_r1009018499
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3089,13 +3089,14 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
submit(finalRdd, Array(0, 1), properties = new Properties())
// Finish the first 2 shuffle map stages.
- completeShuffleMapStageSuccessfully(0, 0, 2)
+ completeShuffleMapStageSuccessfully(0, 0, 2, Seq("hostA", "hostB"))
Review Comment:
This change is not required.
Fetch failed is due to stage 1 partition on hostB going missing - by default, `completeShuffleMapStageSuccessfully` will progressively complete on hostA, hostB, etc ... - it will result in recomputing 0 (since there are two partitions - on hostA and hostB) and 1 (due to fetch failure) - and 2 ofcourse.
In this case, since there is output on hostB for stage 0 (partition 1) and stage 1 (partition 0) , they are recomputed.
If it is confusing, we can add this to the javadoc of `completeShuffleMapStageSuccessfully`
--
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 #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #38371:
URL: https://github.com/apache/spark/pull/38371#discussion_r1009018499
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3089,13 +3089,14 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
submit(finalRdd, Array(0, 1), properties = new Properties())
// Finish the first 2 shuffle map stages.
- completeShuffleMapStageSuccessfully(0, 0, 2)
+ completeShuffleMapStageSuccessfully(0, 0, 2, Seq("hostA", "hostB"))
Review Comment:
This change is not required.
Fetch failed is due to stage 1 partition on hostB going missing - by default, `completeShuffleMapStageSuccessfully` will progressively complete on hostA, hostB, etc ... - it will result in recomputing 0 (since there are two partitions - on hostA and hostB) and 1 (and 2 ofcourse).
In this case, since there is output on hostB for stage 0, it is recomputed.
If it is confusing, we can add this to the javadoc of `completeShuffleMapStageSuccessfully`
--
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] asfgit closed pull request #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
URL: https://github.com/apache/spark/pull/38371
--
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 #38371: Fix some comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #38371:
URL: https://github.com/apache/spark/pull/38371#issuecomment-1289384000
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] mridulm commented on a diff in pull request #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #38371:
URL: https://github.com/apache/spark/pull/38371#discussion_r1009018499
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3089,13 +3089,14 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
submit(finalRdd, Array(0, 1), properties = new Properties())
// Finish the first 2 shuffle map stages.
- completeShuffleMapStageSuccessfully(0, 0, 2)
+ completeShuffleMapStageSuccessfully(0, 0, 2, Seq("hostA", "hostB"))
Review Comment:
This change is not required.
Fetch failed is due to stage 1 partition on hostB going missing - it does not matter where stage 0 ran - it will result in recomputing 1 and 2
--
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] JiexingLi commented on a diff in pull request #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
JiexingLi commented on code in PR #38371:
URL: https://github.com/apache/spark/pull/38371#discussion_r1009014221
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3089,13 +3089,14 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
submit(finalRdd, Array(0, 1), properties = new Properties())
// Finish the first 2 shuffle map stages.
- completeShuffleMapStageSuccessfully(0, 0, 2)
+ completeShuffleMapStageSuccessfully(0, 0, 2, Seq("hostA", "hostB"))
Review Comment:
Yes. The background is that: Wenchen and I were reading the related code, but felt confused about why line 3098 says: "Executor lost on hostB, both of stage 0 and 1 should be reran.".
After digging around, we found that stage0 completes in "hostA", "hostB" (the default hosts somewhere in the code). But it is not very obvious for reader to tell this.
So adding Seq("hostA", "hostB") to reader to easily get this. I further updated the comments too.
--
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 #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #38371:
URL: https://github.com/apache/spark/pull/38371#discussion_r1009024542
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3207,7 +3209,7 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
assert(failure == null, "job should not fail")
val failedStages = scheduler.failedStages.toSeq
assert(failedStages.length == 2)
- // Shuffle blocks of "hostA" is lost, so first task of the `shuffleMapRdd2` needs to retry.
+ // Shuffle blocks of "hostA" is lost, so first task of the `finalRdd` needs to retry.
Review Comment:
`mapRdd` actually.
--
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 #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #38371:
URL: https://github.com/apache/spark/pull/38371#discussion_r1009018882
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3089,13 +3089,14 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
submit(finalRdd, Array(0, 1), properties = new Properties())
// Finish the first 2 shuffle map stages.
- completeShuffleMapStageSuccessfully(0, 0, 2)
+ completeShuffleMapStageSuccessfully(0, 0, 2, Seq("hostA", "hostB"))
assert(mapOutputTracker.findMissingPartitions(shuffleId1) === Some(Seq.empty))
completeShuffleMapStageSuccessfully(1, 0, 2, Seq("hostB", "hostD"))
assert(mapOutputTracker.findMissingPartitions(shuffleId2) === Some(Seq.empty))
- // Executor lost on hostB, both of stage 0 and 1 should be reran.
+ // FetchFailed on stage 2, both of stage 1 and 2 should be reran. Besides, executor lost on
+ // hostB, both of stage 0 and 1 should be reran.
Review Comment:
The comment could read - "Executor lost on hostB, both of stage 0 (if missing partitions) and 1 should be reran - as part of recomputation of stage 2" : if no output on hostB for stage 0 (which appears to be the case), we can omit that from the list of recomputed stages in the comment.
Something like "Executor lost on hostB, both of stage 1 and 2 should be reran."
Will that clarify ?
--
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 #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #38371:
URL: https://github.com/apache/spark/pull/38371#discussion_r1009018499
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3089,13 +3089,14 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
submit(finalRdd, Array(0, 1), properties = new Properties())
// Finish the first 2 shuffle map stages.
- completeShuffleMapStageSuccessfully(0, 0, 2)
+ completeShuffleMapStageSuccessfully(0, 0, 2, Seq("hostA", "hostB"))
Review Comment:
This change is not required.
Fetch failed is due to stage 1 partition on hostB going missing - by default, `completeShuffleMapStageSuccessfully` will progressively complete on hostA, hostB, etc ... - it will result in recomputing 0 (since there are two partitions - on hostA and hostB) and 1 (due to fetch failure) - and 2 ofcourse.
In this case, since there is output on hostB for stage 0 and 1, they are recomputed.
If it is confusing, we can add this to the javadoc of `completeShuffleMapStageSuccessfully`
--
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 #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #38371:
URL: https://github.com/apache/spark/pull/38371#discussion_r1010007062
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3089,13 +3089,14 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
submit(finalRdd, Array(0, 1), properties = new Properties())
// Finish the first 2 shuffle map stages.
- completeShuffleMapStageSuccessfully(0, 0, 2)
+ completeShuffleMapStageSuccessfully(0, 0, 2, Seq("hostA", "hostB"))
Review Comment:
Enhancing documentation of `completeShuffleMapStageSuccessfully` is sufficient - we dont want to explicitly specify `hostNames` for all invocations of the method.
--
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 #38371: Fix some comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on PR #38371:
URL: https://github.com/apache/spark/pull/38371#issuecomment-1288700659
@JiexingLi please keep the PR desscription template (https://github.com/apache/spark/blob/master/.github/PULL_REQUEST_TEMPLATE) and keep the PR title formtted like others (see also https://spark.apache.org/contributing.html)
--
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] itholic commented on pull request #38371: Fix some comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
itholic commented on PR #38371:
URL: https://github.com/apache/spark/pull/38371#issuecomment-1294685551
@JiexingLi Can you address https://github.com/apache/spark/pull/38371#issuecomment-1288700659 when you find some time? Just for reminder, take your time.
--
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 #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #38371:
URL: https://github.com/apache/spark/pull/38371#discussion_r1009018882
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3089,13 +3089,14 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
submit(finalRdd, Array(0, 1), properties = new Properties())
// Finish the first 2 shuffle map stages.
- completeShuffleMapStageSuccessfully(0, 0, 2)
+ completeShuffleMapStageSuccessfully(0, 0, 2, Seq("hostA", "hostB"))
assert(mapOutputTracker.findMissingPartitions(shuffleId1) === Some(Seq.empty))
completeShuffleMapStageSuccessfully(1, 0, 2, Seq("hostB", "hostD"))
assert(mapOutputTracker.findMissingPartitions(shuffleId2) === Some(Seq.empty))
- // Executor lost on hostB, both of stage 0 and 1 should be reran.
+ // FetchFailed on stage 2, both of stage 1 and 2 should be reran. Besides, executor lost on
+ // hostB, both of stage 0 and 1 should be reran.
Review Comment:
The comment should read - "Executor lost on hostB, both of stage 0 and 1 should be reran - as part of recomputation of stage 2"
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3089,13 +3089,14 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
submit(finalRdd, Array(0, 1), properties = new Properties())
// Finish the first 2 shuffle map stages.
- completeShuffleMapStageSuccessfully(0, 0, 2)
+ completeShuffleMapStageSuccessfully(0, 0, 2, Seq("hostA", "hostB"))
assert(mapOutputTracker.findMissingPartitions(shuffleId1) === Some(Seq.empty))
completeShuffleMapStageSuccessfully(1, 0, 2, Seq("hostB", "hostD"))
assert(mapOutputTracker.findMissingPartitions(shuffleId2) === Some(Seq.empty))
- // Executor lost on hostB, both of stage 0 and 1 should be reran.
+ // FetchFailed on stage 2, both of stage 1 and 2 should be reran. Besides, executor lost on
+ // hostB, both of stage 0 and 1 should be reran.
Review Comment:
The comment should read - "Executor lost on hostB, both of stage 0 and 1 should be reran - as part of recomputation of stage 2" : but looks fine to me as-is.
--
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 #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #38371:
URL: https://github.com/apache/spark/pull/38371#discussion_r1009018882
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3089,13 +3089,14 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
submit(finalRdd, Array(0, 1), properties = new Properties())
// Finish the first 2 shuffle map stages.
- completeShuffleMapStageSuccessfully(0, 0, 2)
+ completeShuffleMapStageSuccessfully(0, 0, 2, Seq("hostA", "hostB"))
assert(mapOutputTracker.findMissingPartitions(shuffleId1) === Some(Seq.empty))
completeShuffleMapStageSuccessfully(1, 0, 2, Seq("hostB", "hostD"))
assert(mapOutputTracker.findMissingPartitions(shuffleId2) === Some(Seq.empty))
- // Executor lost on hostB, both of stage 0 and 1 should be reran.
+ // FetchFailed on stage 2, both of stage 1 and 2 should be reran. Besides, executor lost on
+ // hostB, both of stage 0 and 1 should be reran.
Review Comment:
The comment could read - "Executor lost on hostB, both of stage 0 (if missing partitions) and 1 should be reran - as part of recomputation of stage 2" : but looks fine to me as-is.
--
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 #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #38371:
URL: https://github.com/apache/spark/pull/38371#discussion_r1009018882
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3089,13 +3089,14 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
submit(finalRdd, Array(0, 1), properties = new Properties())
// Finish the first 2 shuffle map stages.
- completeShuffleMapStageSuccessfully(0, 0, 2)
+ completeShuffleMapStageSuccessfully(0, 0, 2, Seq("hostA", "hostB"))
assert(mapOutputTracker.findMissingPartitions(shuffleId1) === Some(Seq.empty))
completeShuffleMapStageSuccessfully(1, 0, 2, Seq("hostB", "hostD"))
assert(mapOutputTracker.findMissingPartitions(shuffleId2) === Some(Seq.empty))
- // Executor lost on hostB, both of stage 0 and 1 should be reran.
+ // FetchFailed on stage 2, both of stage 1 and 2 should be reran. Besides, executor lost on
+ // hostB, both of stage 0 and 1 should be reran.
Review Comment:
The comment should read - "Executor lost on hostB, both of stage 1 and 2 should be reran"
--
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 #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #38371:
URL: https://github.com/apache/spark/pull/38371#discussion_r1009018499
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3089,13 +3089,14 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
submit(finalRdd, Array(0, 1), properties = new Properties())
// Finish the first 2 shuffle map stages.
- completeShuffleMapStageSuccessfully(0, 0, 2)
+ completeShuffleMapStageSuccessfully(0, 0, 2, Seq("hostA", "hostB"))
Review Comment:
This change is not required.
Fetch failed is due to stage 1 partition on hostB going missing - it does not matter where stage 0 ran - it will result in recomputing 0 (if it has missing output) and 1 (and 2 ofcourse).
In this case, since there is no output on hostB for stage 0, it is not recomputed.
--
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 #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #38371:
URL: https://github.com/apache/spark/pull/38371#discussion_r1009018882
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3089,13 +3089,14 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
submit(finalRdd, Array(0, 1), properties = new Properties())
// Finish the first 2 shuffle map stages.
- completeShuffleMapStageSuccessfully(0, 0, 2)
+ completeShuffleMapStageSuccessfully(0, 0, 2, Seq("hostA", "hostB"))
assert(mapOutputTracker.findMissingPartitions(shuffleId1) === Some(Seq.empty))
completeShuffleMapStageSuccessfully(1, 0, 2, Seq("hostB", "hostD"))
assert(mapOutputTracker.findMissingPartitions(shuffleId2) === Some(Seq.empty))
- // Executor lost on hostB, both of stage 0 and 1 should be reran.
+ // FetchFailed on stage 2, both of stage 1 and 2 should be reran. Besides, executor lost on
+ // hostB, both of stage 0 and 1 should be reran.
Review Comment:
The comment could read - "Executor lost on hostB, both of stage 0 and 1 should be reran - as part of recomputation of stage 2" : since there is output on hostB for stage 0 (in completeShuffleMapStageSuccessfully), and stage 1.
Something like "Executor lost on hostB, both of stage 0, 1 and 2 should be reran due to missing outputs"
Will that clarify ?
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3089,13 +3089,14 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
submit(finalRdd, Array(0, 1), properties = new Properties())
// Finish the first 2 shuffle map stages.
- completeShuffleMapStageSuccessfully(0, 0, 2)
+ completeShuffleMapStageSuccessfully(0, 0, 2, Seq("hostA", "hostB"))
assert(mapOutputTracker.findMissingPartitions(shuffleId1) === Some(Seq.empty))
completeShuffleMapStageSuccessfully(1, 0, 2, Seq("hostB", "hostD"))
assert(mapOutputTracker.findMissingPartitions(shuffleId2) === Some(Seq.empty))
- // Executor lost on hostB, both of stage 0 and 1 should be reran.
+ // FetchFailed on stage 2, both of stage 1 and 2 should be reran. Besides, executor lost on
+ // hostB, both of stage 0 and 1 should be reran.
Review Comment:
The comment could read - "Executor lost on hostB, both of stage 0 and 1 should be reran - as part of recomputation of stage 2" : since there is output on hostB for stage 0 (in completeShuffleMapStageSuccessfully), and stage 1.
Something like "Executor lost on hostB, both of stage 0, 1 and 2 should be reran due to missing partitions"
Will that clarify ?
--
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 #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #38371:
URL: https://github.com/apache/spark/pull/38371#discussion_r1009018882
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3089,13 +3089,14 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
submit(finalRdd, Array(0, 1), properties = new Properties())
// Finish the first 2 shuffle map stages.
- completeShuffleMapStageSuccessfully(0, 0, 2)
+ completeShuffleMapStageSuccessfully(0, 0, 2, Seq("hostA", "hostB"))
assert(mapOutputTracker.findMissingPartitions(shuffleId1) === Some(Seq.empty))
completeShuffleMapStageSuccessfully(1, 0, 2, Seq("hostB", "hostD"))
assert(mapOutputTracker.findMissingPartitions(shuffleId2) === Some(Seq.empty))
- // Executor lost on hostB, both of stage 0 and 1 should be reran.
+ // FetchFailed on stage 2, both of stage 1 and 2 should be reran. Besides, executor lost on
+ // hostB, both of stage 0 and 1 should be reran.
Review Comment:
The comment could read - "Executor lost on hostB, both of stage 0 and 1 should be rerun - as part of recomputation of stage 2" : since there is output on hostB for stage 0 (in completeShuffleMapStageSuccessfully), and stage 1.
Will that clarify ?
--
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 #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #38371:
URL: https://github.com/apache/spark/pull/38371#discussion_r1009018882
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3089,13 +3089,14 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
submit(finalRdd, Array(0, 1), properties = new Properties())
// Finish the first 2 shuffle map stages.
- completeShuffleMapStageSuccessfully(0, 0, 2)
+ completeShuffleMapStageSuccessfully(0, 0, 2, Seq("hostA", "hostB"))
assert(mapOutputTracker.findMissingPartitions(shuffleId1) === Some(Seq.empty))
completeShuffleMapStageSuccessfully(1, 0, 2, Seq("hostB", "hostD"))
assert(mapOutputTracker.findMissingPartitions(shuffleId2) === Some(Seq.empty))
- // Executor lost on hostB, both of stage 0 and 1 should be reran.
+ // FetchFailed on stage 2, both of stage 1 and 2 should be reran. Besides, executor lost on
+ // hostB, both of stage 0 and 1 should be reran.
Review Comment:
This comment change is not required.
--
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 #38371: [SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #38371:
URL: https://github.com/apache/spark/pull/38371#discussion_r1009018499
##########
core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala:
##########
@@ -3089,13 +3089,14 @@ class DAGSchedulerSuite extends SparkFunSuite with TempLocalSparkContext with Ti
submit(finalRdd, Array(0, 1), properties = new Properties())
// Finish the first 2 shuffle map stages.
- completeShuffleMapStageSuccessfully(0, 0, 2)
+ completeShuffleMapStageSuccessfully(0, 0, 2, Seq("hostA", "hostB"))
Review Comment:
This change is not required.
Fetch failed is due to stage 1 partition on hostB going missing - it does not matter where stage 0 ran - it will be recomputed for indeterminate stages.
--
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