[SPARK-40968] Fix a few wrong/misleading comments in DAGSchedulerSuite

[JiexingLi]

What changes were proposed in this pull request?

Fix a few wrong or misleading comments in DAGSchedulerSuite.

Why are the changes needed?

The wrong or misleading comments in DAGSchedulerSuite introduce confusions and make it harder to understanding the code.

Does this PR introduce any user-facing change?

No

How was this patch tested?

No code changes, pure comment changes.
Original tests pass.

[HyukjinKwon]

@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)

[AmplabJenkins]

Can one of the admins verify this patch?

[itholic]

@JiexingLi Can you address #38371 (comment) when you find some time? Just for reminder, take your time.

[JiexingLi]

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.

[HyukjinKwon]

This not only fixes the comment but the test codes. Mind elaborating this please?

[JiexingLi]

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.

[mridulm]

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

[JiexingLi]

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().

[mridulm]

Enhancing documentation of completeShuffleMapStageSuccessfully is sufficient - we dont want to explicitly specify hostNames for all invocations of the method.

[JiexingLi]

Sounds good. Deleted the hostNames param.

[mridulm]

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.

[JiexingLi]

Thanks, Mridul.

I updated the comment. Your rewrite is very neat and clear.

[mridulm]

mapRdd actually.

[JiexingLi]

Yes, my bad.

Thanks and updated.

[mridulm]

Merged to master, thanks or fixing this @JiexingLi !
Thanks for looking into this @HyukjinKwon :-)