HyukjinKwon commented on a change in pull request #33644:
URL: https://github.com/apache/spark/pull/33644#discussion_r684855590
##########
File path: core/src/main/scala/org/apache/spark/internal/config/package.scala
##########
@@ -315,6 +315,12 @@ package object config {
.bytesConf(ByteUnit.MiB)
.createOptional
+ private[spark] val ENABLE_EXECUTOR_TREE_AGGREGATE =
ConfigBuilder("spark.executor.treeAggregate")
Review comment:
I have a similar feeling w/ Sean's comment here. Should we maybe just
add it as a parameter instead of adding it as a configuration? Not sure which
way is the best. One concern might be that this is a static configuration that
cannot be changed.
##########
File path: core/src/test/scala/org/apache/spark/rdd/RDDSuite.scala
##########
@@ -275,6 +275,18 @@ class RDDSuite extends SparkFunSuite with
SharedSparkContext with Eventually {
}
}
+ test("treeAggregate with ENABLE_EXECUTOR_TREE_AGGREGATE true") {
Review comment:
nit .. but I would add a JIRA prefix
##########
File path: core/src/main/scala/org/apache/spark/internal/config/package.scala
##########
@@ -315,6 +315,12 @@ package object config {
.bytesConf(ByteUnit.MiB)
.createOptional
+ private[spark] val ENABLE_EXECUTOR_TREE_AGGREGATE =
ConfigBuilder("spark.executor.treeAggregate")
Review comment:
If we will add it as a configuration, I would rename it something like
`spark.rdd.treeAggregate.finalAggregateOnExecutor` (feel free to change the
wording).
##########
File path: core/src/test/scala/org/apache/spark/rdd/RDDSuite.scala
##########
@@ -275,6 +275,18 @@ class RDDSuite extends SparkFunSuite with
SharedSparkContext with Eventually {
}
}
+ test("treeAggregate with ENABLE_EXECUTOR_TREE_AGGREGATE true") {
Review comment:
and should probably fix the test title
(`ENABLE_EXECUTOR_TREE_AGGREGATE`) too.
##########
File path: core/src/main/scala/org/apache/spark/internal/config/package.scala
##########
@@ -315,6 +315,12 @@ package object config {
.bytesConf(ByteUnit.MiB)
.createOptional
+ private[spark] val ENABLE_EXECUTOR_TREE_AGGREGATE =
ConfigBuilder("spark.executor.treeAggregate")
Review comment:
(If we'd add it as a parameter, we will have to add it to `JavaRDDLike`
too)
--
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]
