Ngone51 commented on a change in pull request #31643:
URL: https://github.com/apache/spark/pull/31643#discussion_r584025854
##########
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:
Ah, that's a good point. I agree the assertion is not necessary. But if
we assume the random `blockIds` here, the current fix won't work with duplicate
`mapIds`. (we should be able to save duplicate `mapIds` in `FetchShuffleBlocks`
to ensure the server side serves chunks in the same order with `blockIds` )
----------------------------------------------------------------
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
