[SPARK-48631][CORE][TEST] Fix test "error during accessing host local dirs for executors" #46989
Conversation
|
@attilapiros, could you take a look at this? |
| configureMockTransfer(Map()) | ||
| val iterator = createShuffleBlockIteratorWithDefaults( | ||
| Map(hostLocalBmId -> toBlockList(hostLocalBlocks.keys, 1L, 1)) | ||
| Map(blockManager.blockManagerId -> toBlockList(hostLocalBlocks.keys, 1L, 1)) |
There was a problem hiding this comment.
Wounldn't it be treated as local blocks rather than host-local blocks?
There was a problem hiding this comment.
That's correct. Thanks for catching!
It seems that for the original code, blockManager.hostLocalDirManager was not mocked correctly and it returns None. I'm trying to figure out how to mock it correctly.
There was a problem hiding this comment.
Fixed the test properly... Do you mind taking a look again?
| val iterator = createShuffleBlockIteratorWithDefaults( | ||
| Map(hostLocalBmId -> toBlockList(hostLocalBlocks.keys, 1L, 1)) | ||
| Map(hostLocalBmId -> toBlockList(hostLocalBlocks.keys, 1L, 1)), | ||
| blockManager = Some(blockManager) |
There was a problem hiding this comment.
blockManager is actually the default value of createShuffleBlockIteratorWithDefaults(). So it looks like there is no actual difference made by this change.
There was a problem hiding this comment.
We do change the return of blockManager.hostLocalDirManager to Some(hostLocalDirManager), so that the blocks are treated as host-local.
There was a problem hiding this comment.
Verified locally that the correct code path was reached in partitionBlocksByFetchMode. I can add an additional check to verify that mockExternalBlockStoreClient.getHostLocalDirs was called in the test.
|
Thanks, merged to master! |
3 participants
What changes were proposed in this pull request?
This change fixes test "error during accessing host local dirs for executors" in ShuffleBlockFetcherIteratorSuite, by setting up
blocksByAddresscorrectly for theShuffleBlockFetcherIteratorused in the test.Why are the changes needed?
The test is for host local blocks, but the
BlockManagerIdwas not set up correctly, soShuffleBlockFetcherIteratorwould treat those blocks as remote blocks instead.This change is to make the test valid to prevent further breaking changes.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Added an additional check in the test. Also tested locally and verified that the correct code path was reached.
Was this patch authored or co-authored using generative AI tooling?
No.