sunchao commented on PR #58402: URL: https://github.com/apache/spark/pull/58402#issuecomment-5459746930
Thanks for the detailed review, @dongjoon-hyun. I updated the comments and PR description. 1. The rollback interaction is fixed in [78f741b](https://github.com/sunchao/spark/commit/78f741b941d9e629d308f8e4d1ec66aba3f008b2). Barrier failures now use a separate `maxFailedBarrierAttemptId`, leaving `maxAttemptIdToIgnore` and rollback deduplication unchanged. The skipped cleanup can also matter today if a delayed shuffle-migration report restores an output before rollback cleanup. I added regressions for both ordinary task failure and `FetchFailed`: both pass on the public baseline, fail on the earlier shared-marker revision, and pass on the revised patch. `markAsRollingBack()` remains specific to rollback, so its existing method documentation still applies. 2. The producer-side `unregisterAllMapAndMergeOutput()` path is unchanged. The description now explicitly says that this PR does not protect already accepted retry outputs from a later downstream failure or add attempt validation to asynchronous migration updates. The producer-retry test checks that subsequent successes remain eligible, not that previously registered outputs survive clearing. For the particular completed-producer straggler scenario, I have not established a reachable ordering through normal scheduling: barrier speculation is disabled, normal stage completion processes all pending successes, duplicate terminal reports are dropped, and older failed or rolled-back attempts are already filtered. A concrete event ordering would help establish a separate regression before adding producer invalidation. This does not claim to resolve the broader producer-side recovery races. 3. I restored the explanation for clearing every map output at both failure sites, alongside the invalidation comment. This follow-up changes only code comments and the PR description. Scalastyle and whitespace checks passed; scheduler and native tests were not rerun for the comment-only change. The description retains the earlier 354 scheduler tests and six native local scenarios, with their tested commit and coverage limits. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
