Ngone51 commented on a change in pull request #33617:
URL: https://github.com/apache/spark/pull/33617#discussion_r687372545
##########
File path:
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java
##########
@@ -514,12 +512,12 @@ public MergeStatuses
finalizeShuffleMerge(FinalizeShuffleMerge msg) {
AppShuffleInfo appShuffleInfo = validateAndGetAppShuffleInfo(msg.appId);
if (appShuffleInfo.attemptId != msg.appAttemptId) {
// If this Block belongs to a former application attempt, it is
considered late,
- // as only the blocks from the current application attempt will be merged
- // TODO: [SPARK-35548] Client should be updated to handle this error.
- throw new IllegalArgumentException(
- String.format("The attempt id %s in this FinalizeShuffleMerge message
does not match "
- + "with the current attempt id %s stored in shuffle service for
application %s",
- msg.appAttemptId, appShuffleInfo.attemptId, msg.appId));
+ // as only the blocks from the current application attempt will be
merged.
+ // No need use callback to return to client now, because the
finalizeShuffleMerge
+ // in DAGScheduler has no retry policy.
Review comment:
> Given the other comment from @mridulm about too old attempt only being
seen by an already failed app attempt, I also don't feel that it's necessary to
do the handling I mentioned earlier.
I was thinking about this too. So it might be ok to not handle it for
simiplity.
@zhuqi-lucas Sorry for the bother. Could you revert this part change?
##########
File path:
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java
##########
@@ -514,12 +512,12 @@ public MergeStatuses
finalizeShuffleMerge(FinalizeShuffleMerge msg) {
AppShuffleInfo appShuffleInfo = validateAndGetAppShuffleInfo(msg.appId);
if (appShuffleInfo.attemptId != msg.appAttemptId) {
// If this Block belongs to a former application attempt, it is
considered late,
- // as only the blocks from the current application attempt will be merged
- // TODO: [SPARK-35548] Client should be updated to handle this error.
- throw new IllegalArgumentException(
- String.format("The attempt id %s in this FinalizeShuffleMerge message
does not match "
- + "with the current attempt id %s stored in shuffle service for
application %s",
- msg.appAttemptId, appShuffleInfo.attemptId, msg.appId));
+ // as only the blocks from the current application attempt will be
merged.
+ // No need use callback to return to client now, because the
finalizeShuffleMerge
+ // in DAGScheduler has no retry policy.
Review comment:
> Given the other comment from @mridulm about too old attempt only being
seen by an already failed app attempt, I also don't feel that it's necessary to
do the handling I mentioned earlier.
I was thinking about this too. So it might be ok to not handle it for
simplicity.
@zhuqi-lucas Sorry for the bother. Could you revert this part 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: [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]
