otterc commented on a change in pull request #31643:
URL: https://github.com/apache/spark/pull/31643#discussion_r583790528
##########
File path:
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/OneForOneBlockFetcher.java
##########
@@ -117,17 +117,23 @@ private FetchShuffleBlocks createFetchShuffleBlocksMsg(
boolean batchFetchEnabled = firstBlock.length == 5;
HashMap<Long, ArrayList<Integer>> mapIdToReduceIds = new HashMap<>();
+ ArrayList<Long> orderedMapId = new ArrayList<>();
for (String blockId : blockIds) {
String[] blockIdParts = splitBlockId(blockId);
if (Integer.parseInt(blockIdParts[1]) != shuffleId) {
throw new IllegalArgumentException("Expected shuffleId=" + shuffleId +
", got:" + blockId);
}
long mapId = Long.parseLong(blockIdParts[2]);
+ assert(orderedMapId.isEmpty() || mapId >=
orderedMapId.get(orderedMapId.size() - 1));
Review comment:
I actually don't think that the bug reveals that order in blockIds is
important. Just in `OneForOneBlockFetcher`, it never expects the provided
`blockIds` are in certain order. Also at this layer why should it expect that?
It is given an array of blockIds which may be in any order and would fetch
them.
IMO, the bug is just that the member `blockIds` of OneForOneBlockFetcher is
out of sync with the FetchShuffleBlocks.
----------------------------------------------------------------
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
