-
Notifications
You must be signed in to change notification settings - Fork 141
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[MINOR] improvement: Reduce the size of Spark patch #699
Conversation
Codecov Report
@@ Coverage Diff @@
## master #699 +/- ##
============================================
+ Coverage 60.80% 63.06% +2.26%
- Complexity 1840 1841 +1
============================================
Files 221 207 -14
Lines 12648 10683 -1965
Branches 1062 1062
============================================
- Hits 7690 6737 -953
+ Misses 4552 3601 -951
+ Partials 406 345 -61 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@@ -188,6 +188,8 @@ public RssShuffleManager(SparkConf sparkConf, boolean isDriver) { | |||
// External shuffle service is not supported when using remote shuffle service | |||
sparkConf.set("spark.shuffle.service.enabled", "false"); | |||
LOG.info("Disable external shuffle service in RssShuffleManager."); | |||
sparkConf.set("spark.shuffle.reduceLocality.enabled", "false"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#341 need this feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because our data is in the RSS cluster or HDFS, locatity is useless for us. If rack aware need this feature, we can turn on this feature when we use rack aware.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. The locality awareness could be supported in the future.
@@ -188,6 +188,8 @@ public RssShuffleManager(SparkConf sparkConf, boolean isDriver) { | |||
// External shuffle service is not supported when using remote shuffle service | |||
sparkConf.set("spark.shuffle.service.enabled", "false"); | |||
LOG.info("Disable external shuffle service in RssShuffleManager."); | |||
sparkConf.set("spark.shuffle.reduceLocality.enabled", "false"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind add a comment here, like L188.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
@@ -205,6 +205,8 @@ public RssShuffleManager(SparkConf conf, boolean isDriver) { | |||
LOG.info("Disable external shuffle service in RssShuffleManager."); | |||
sparkConf.set("spark.sql.adaptive.localShuffleReader.enabled", "false"); | |||
LOG.info("Disable local shuffle reader in RssShuffleManager."); | |||
sparkConf.set("spark.shuffle.reduceLocality.enabled", "false"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, except two minor comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
### What changes were proposed in this pull request? As apache/spark#40307 (comment), we could use `spark.shuffle.reduceLocality.enabled` to reduce the modification of the Apache Spark. ### Why are the changes needed? Reduce the spark patch size ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? No need Co-authored-by: roryqi <roryqi@tencent.com>
### What changes were proposed in this pull request? As apache/spark#40307 (comment), we could use `spark.shuffle.reduceLocality.enabled` to reduce the modification of the Apache Spark. ### Why are the changes needed? Reduce the spark patch size ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? No need Co-authored-by: roryqi <roryqi@tencent.com>
What changes were proposed in this pull request?
As apache/spark#40307 (comment), we could use
spark.shuffle.reduceLocality.enabled
to reduce the modification of the Apache Spark.Why are the changes needed?
Reduce the spark patch size
Does this PR introduce any user-facing change?
No.
How was this patch tested?
No need