You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "hiboyang (via GitHub)" <gi...@apache.org> on 2024/04/02 00:42:59 UTC
[PR] [SHUFFLE] [SPARK-47678] Check `spark.shuffle.readHostLocalDisk` when reading shuffle blocks [spark]
hiboyang opened a new pull request, #45803:
URL: https://github.com/apache/spark/pull/45803
### What changes were proposed in this pull request?
Check `spark.shuffle.readHostLocalDisk` config to determine whether read shuffle block from same local host machine.
### Why are the changes needed?
Spark has a special shuffle optimization to check whether the shuffle block is on the same local host machine, and read from local disk when possible. This local host machine check is done by comparing block ip address with current host ip address. This will cause issue when running Spark on Kubernetes, because Kubernetes may reuse pod ip when some old executor exits and the new executor starts.
Consider following sequence:
1. Executor 1 starts with IP address 10.0.0.1.
2. Some shuffle block (e.g. block1) is written on Executor 1.
3. Executor 1 terminates.
4. Executor 2 starts with same IP address 10.0.0.1 (this is rare, but did happen in our test, because Kubernetes may reuse ip when launching pods).
5. Executor 2 tries to read block1. It finds block1's address is same as current host address, thus assumes block1 exists on its local disk.
6. Executor 2 will read from local disk and get error, since block1 is not there (block1 is on Executor 1, which is gone)
This is already a Spark config (spark.shuffle.readHostLocalDisk). We can reuse this config and check it in BlockStoreShuffleReader.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Manually tested.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47678][CORE] Check `spark.shuffle.readHostLocalDisk` when reading shuffle blocks [spark]
Posted by "hiboyang (via GitHub)" <gi...@apache.org>.
hiboyang commented on code in PR #45803:
URL: https://github.com/apache/spark/pull/45803#discussion_r1566498567
##########
core/src/main/scala/org/apache/spark/storage/ShuffleBlockFetcherIterator.scala:
##########
@@ -417,7 +418,7 @@ final class ShuffleBlockFetcherIterator(
numBlocksToFetch += mergedBlockInfos.size
localBlocks ++= mergedBlockInfos.map(info => (info.blockId, info.mapIndex))
localBlockBytes += mergedBlockInfos.map(_.size).sum
- } else if (blockManager.hostLocalDirManager.isDefined &&
+ } else if (doLocalDiskRead && blockManager.hostLocalDirManager.isDefined &&
Review Comment:
Did a quick test by disabling `SHUFFLE_HOST_LOCAL_DISK_READING_ENABLED`, looks good. Will close this PR.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47678][CORE] Check `spark.shuffle.readHostLocalDisk` when reading shuffle blocks [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #45803:
URL: https://github.com/apache/spark/pull/45803#issuecomment-2057938423
Thank you @hiboyang !
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47678][CORE] Check `spark.shuffle.readHostLocalDisk` when reading shuffle blocks [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #45803:
URL: https://github.com/apache/spark/pull/45803#discussion_r1548303144
##########
core/src/main/scala/org/apache/spark/storage/ShuffleBlockFetcherIterator.scala:
##########
@@ -417,7 +418,7 @@ final class ShuffleBlockFetcherIterator(
numBlocksToFetch += mergedBlockInfos.size
localBlocks ++= mergedBlockInfos.map(info => (info.blockId, info.mapIndex))
localBlockBytes += mergedBlockInfos.map(_.size).sum
- } else if (blockManager.hostLocalDirManager.isDefined &&
+ } else if (doLocalDiskRead && blockManager.hostLocalDirManager.isDefined &&
Review Comment:
Hmm, if `SHUFFLE_HOST_LOCAL_DISK_READING_ENABLED` is disabled, I think `BlockManager` won't have `hostLocalDirManager` (i.e., it is `None`), except push-based shuffle is enabled.
But I think push-based shuffle is only enabled under YARN mode. For K8s, it is disabled.
How does it enters this branch if `blockManager.hostLocalDirManager.isDefined` is false?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47678][CORE] Check `spark.shuffle.readHostLocalDisk` when reading shuffle blocks [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #45803:
URL: https://github.com/apache/spark/pull/45803#discussion_r1548303144
##########
core/src/main/scala/org/apache/spark/storage/ShuffleBlockFetcherIterator.scala:
##########
@@ -417,7 +418,7 @@ final class ShuffleBlockFetcherIterator(
numBlocksToFetch += mergedBlockInfos.size
localBlocks ++= mergedBlockInfos.map(info => (info.blockId, info.mapIndex))
localBlockBytes += mergedBlockInfos.map(_.size).sum
- } else if (blockManager.hostLocalDirManager.isDefined &&
+ } else if (doLocalDiskRead && blockManager.hostLocalDirManager.isDefined &&
Review Comment:
Hmm, if `SHUFFLE_HOST_LOCAL_DISK_READING_ENABLED` is disabled, I think `BlockManager` won't have `hostLocalDirManager` (i.e., it is `None`), except push-based shuffle is enabled. I.e., this `doLocalDiskRead` condition looks redundant.
But I think push-based shuffle is only enabled under YARN mode. For K8s, it is disabled.
How does it enters this branch if `blockManager.hostLocalDirManager.isDefined` is false?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47678][CORE] Check `spark.shuffle.readHostLocalDisk` when reading shuffle blocks [spark]
Posted by "hiboyang (via GitHub)" <gi...@apache.org>.
hiboyang commented on code in PR #45803:
URL: https://github.com/apache/spark/pull/45803#discussion_r1548363242
##########
core/src/main/scala/org/apache/spark/storage/ShuffleBlockFetcherIterator.scala:
##########
@@ -417,7 +418,7 @@ final class ShuffleBlockFetcherIterator(
numBlocksToFetch += mergedBlockInfos.size
localBlocks ++= mergedBlockInfos.map(info => (info.blockId, info.mapIndex))
localBlockBytes += mergedBlockInfos.map(_.size).sum
- } else if (blockManager.hostLocalDirManager.isDefined &&
+ } else if (doLocalDiskRead && blockManager.hostLocalDirManager.isDefined &&
Review Comment:
Ah, I think you are right! Looks like disabling `SHUFFLE_HOST_LOCAL_DISK_READING_ENABLED` is good enough on K8s. Let me do some more testing.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47678][CORE] Check `spark.shuffle.readHostLocalDisk` when reading shuffle blocks [spark]
Posted by "hiboyang (via GitHub)" <gi...@apache.org>.
hiboyang closed pull request #45803: [SPARK-47678][CORE] Check `spark.shuffle.readHostLocalDisk` when reading shuffle blocks
URL: https://github.com/apache/spark/pull/45803
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org