mridulm commented on a change in pull request #34122:
URL: https://github.com/apache/spark/pull/34122#discussion_r792766502
##########
File path: core/src/main/scala/org/apache/spark/Dependency.scala
##########
@@ -114,15 +114,6 @@ class ShuffleDependency[K: ClassTag, V: ClassTag, C:
ClassTag](
def shuffleMergeAllowed : Boolean = _shuffleMergeAllowed
- // By default, shuffle merge is enabled for ShuffleDependency if
shuffleMergeAllowed
- private[this] var _shuffleMergeEnabled = shuffleMergeAllowed
-
- private[spark] def setShuffleMergeEnabled(shuffleMergeEnabled: Boolean):
Unit = {
- _shuffleMergeEnabled = shuffleMergeEnabled
- }
-
- def shuffleMergeEnabled : Boolean = _shuffleMergeEnabled
Review comment:
Let us keep this method - and make it
`def shuffleMergeEnabled = mergerLocs.nonEmpty`
What I meant was, let us remove the boolean `_shuffleMergeEnabled`
So that rest of the code can depend on this as a state and not need to
inspect the mergerLocs directly
Also, this is part of public api - so cant remove it :-)
Speaking of which, can you make `getFinalizeTask`/`getFinalizeTask`
`private[spark]` as well ? (this or different pr - `ShuffleDependency` is
developer api and we cant leak impl details)
--
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-unsubscr...@spark.apache.org
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
