mridulm commented on a change in pull request #34122:
URL: https://github.com/apache/spark/pull/34122#discussion_r793833288
##########
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:
> We are using mergerLocs.isEmpty in other places as well. If we use
shuffleMergeEnabled (mergerLocs.nonEmpty) and mergerLocs.isEmpty|nonEmpty in
few other places, wouldn't it be confusing?
Yes, we should not blindly replace - only when the intent is to check if
shuffle merge is enabled; though I would expect a empty/nonEmpty check against
mergerLocs to pretty much be a proxy for is shuffle merge enabled.
For example, the changes in the tests - the earlier intent was to check if
merge was enabled; which got replaced with checks against mergerLocs
--
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]
