otterc commented on a change in pull request #32140:
URL: https://github.com/apache/spark/pull/32140#discussion_r645870074
##########
File path:
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/OneForOneBlockFetcher.java
##########
@@ -246,6 +304,14 @@ private void failRemainingBlocks(String[] failedBlockIds,
Throwable e) {
}
}
+ private void failSingleBlockChunk(String shuffleBlockChunkId, Throwable e) {
+ try {
+ listener.onBlockFetchFailure(shuffleBlockChunkId, e);
+ } catch (Exception e2) {
+ logger.error("Error from blockFetchFailure callback", e2);
+ }
+ }
Review comment:
Thanks for bringing this up @mridulm. This is something I missed as the
push-based shuffle code evolved. We don't need this change and I will revert it.
We do want to fail all the remaining chunkIds as well so that they get
retried by `RetryingBlockFetcher`.
Even with this code, the remaining chunkIds are going to be retried by
`RetryingBlockFetcher` because that not only retries the fetch of the failed
block but also the fetch of all the outstanding blocks. In addition
`RetryingBlockFetchListener.initiateRetry` also changes the currentListener so
any invocation of `onBlockFetchFailure()` on an old instance of
`RetryingBlockFetchListener` would do nothing because that checks whether the
instance is the current one or not.
In short, this change is unnecessary here and is not needed.
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
