Victsm commented on a change in pull request #34156:
URL: https://github.com/apache/spark/pull/34156#discussion_r719571404
##########
File path: core/src/main/scala/org/apache/spark/MapOutputTracker.scala
##########
@@ -1448,7 +1448,7 @@ private[spark] object MapOutputTracker extends Logging {
// TODO: improve push based shuffle to read partial merged blocks
satisfying the start/end
// TODO: map indexes
if (mergeStatuses.exists(_.nonEmpty) && startMapIndex == 0
- && endMapIndex == mapStatuses.length) {
+ && endMapIndex == mapStatuses.length && endPartition - startPartition ==
1) {
Review comment:
> For this way, I'm afraid we have to change the interface
ShuffleManager.getReader, which could involve more changes.
I don't think changing the interface to `ShuffleManager.getReader` is needed.
As demonstrated in this PR inside `BlockStoreShuffleReader`, we can disable
batch fetch when push-based shuffle is enabled and it's an equally simple fix.
The default configs in 3.2.0 will initiate partition coalesce if the size
per shuffle partition is less than 1MB, and people might want to enable
partition coalesce to fix the many small files problems.
These valid reasons where partition coalesce is preferred to be enabled
might make it even trickier to reason with when push-based shuffle actually
gets enabled.
Block batching and push-based shuffle could potentially work together, i.e.
we could batch blocks together to be pushed to create merged shuffle files that
span across multiple shuffle partitions and enable push-based shuffle and batch
fetching to properly work together.
However, for the sake of unblocking 3.2.0 release, we need a much quicker
resolution to this issue.
For that, when the flags for AQE, partition coalesce, batch fetch, and
push-based shuffle are all set to true, we will have to either disable
push-based shuffle and leave batch fetch as is with partition coalesce, or we
can disable batch fetch and leave push-based shuffle as is with partition
coalesce.
I prefer the 2nd option, because it makes it more consistent for enabling
the flag of push-based shuffle and push-based shuffle is a more thorough
solution for the problem that batch fetch is trying to solve.
##########
File path: core/src/main/scala/org/apache/spark/MapOutputTracker.scala
##########
@@ -1448,7 +1448,7 @@ private[spark] object MapOutputTracker extends Logging {
// TODO: improve push based shuffle to read partial merged blocks
satisfying the start/end
// TODO: map indexes
if (mergeStatuses.exists(_.nonEmpty) && startMapIndex == 0
- && endMapIndex == mapStatuses.length) {
+ && endMapIndex == mapStatuses.length && endPartition - startPartition ==
1) {
Review comment:
> For this way, I'm afraid we have to change the interface
ShuffleManager.getReader, which could involve more changes.
I don't think changing the interface to `ShuffleManager.getReader` is needed.
As demonstrated in this PR inside `BlockStoreShuffleReader`, we can disable
batch fetch when push-based shuffle is enabled and it's an equally simple fix.
The default configs in 3.2.0 will initiate partition coalesce if the size
per shuffle partition is less than 1MB, and people might want to enable
partition coalesce to fix the many small files problems.
These valid reasons where partition coalesce is preferred to be enabled
might make it even trickier to reason with when push-based shuffle actually
gets enabled.
Block batching and push-based shuffle could potentially work together, i.e.
we could batch blocks together to be pushed to create merged shuffle files that
span across multiple shuffle partitions and enable push-based shuffle and batch
fetching to properly work together.
However, for the sake of unblocking 3.2.0 release, we need a much quicker
resolution to this issue.
For that, when the flags for AQE, partition coalesce, batch fetch, and
push-based shuffle are all set to true, we will have to either disable
push-based shuffle and leave batch fetch as is with partition coalesce, or we
can disable batch fetch and leave push-based shuffle as is with partition
coalesce.
I prefer the 2nd option, because it makes it more consistent for enabling
the flag of push-based shuffle and push-based shuffle is a more thorough
solution for the problem that batch fetch is trying to solve.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
