otterc commented on a change in pull request #32140: URL: https://github.com/apache/spark/pull/32140#discussion_r649419059
########## File path: core/src/main/scala/org/apache/spark/storage/ShuffleBlockFetcherIterator.scala ########## @@ -347,77 +355,118 @@ final class ShuffleBlockFetcherIterator( } } - private[this] def partitionBlocksByFetchMode(): ArrayBuffer[FetchRequest] = { + /** + * This is called from initialize and also from the fallback which is triggered from + * [[PushBasedFetchHelper]]. + */ + private[this] def partitionBlocksByFetchMode( + blocksByAddress: Iterator[(BlockManagerId, Seq[(BlockId, Long, Int)])], + localBlocks: mutable.LinkedHashSet[(BlockId, Int)], + hostLocalBlocksByExecutor: mutable.LinkedHashMap[BlockManagerId, Seq[(BlockId, Long, Int)]], + mergedLocalBlocks: mutable.LinkedHashSet[BlockId]): ArrayBuffer[FetchRequest] = { logDebug(s"maxBytesInFlight: $maxBytesInFlight, targetRemoteRequestSize: " + s"$targetRemoteRequestSize, maxBlocksInFlightPerAddress: $maxBlocksInFlightPerAddress") - // Partition to local, host-local and remote blocks. Remote blocks are further split into - // FetchRequests of size at most maxBytesInFlight in order to limit the amount of data in flight + // Partition to local, host-local, merged-local, remote (includes merged-remote) blocks. + // Remote blocks are further split into FetchRequests of size at most maxBytesInFlight in order + // to limit the amount of data in flight val collectedRemoteRequests = new ArrayBuffer[FetchRequest] + val hostLocalBlocksCurrentIteration = mutable.LinkedHashSet[(BlockId, Int)]() var localBlockBytes = 0L var hostLocalBlockBytes = 0L + var mergedLocalBlockBytes = 0L var remoteBlockBytes = 0L + val prevNumBlocksToFetch = numBlocksToFetch val fallback = FallbackStorage.FALLBACK_BLOCK_MANAGER_ID.executorId for ((address, blockInfos) <- blocksByAddress) { - if (Seq(blockManager.blockManagerId.executorId, fallback).contains(address.executorId)) { - checkBlockSizes(blockInfos) Review comment: @mridulm it was done earlier for the last `else` as well but it was part of `collectFetchRequests`. Based on this conversation https://github.com/apache/spark/pull/32140#discussion_r648301802, I moved it before the `if/else if`. I also removed it from `collectFetchRequests` so that for remote blocks, it's not done twice. -- 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