-
Notifications
You must be signed in to change notification settings - Fork 151
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] fix(spark-client): Fix fallback to sort fail issue #2329
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2329 +/- ##
============================================
+ Coverage 51.19% 52.01% +0.81%
- Complexity 2936 2938 +2
============================================
Files 486 465 -21
Lines 24720 22181 -2539
Branches 2041 2041
============================================
- Hits 12656 11537 -1119
+ Misses 11274 9928 -1346
+ Partials 790 716 -74 ☔ View full report in Codecov by Sentry. |
@@ -104,6 +104,7 @@ private ShuffleManager createShuffleManagerInDriver() throws RssException { | |||
Constants.SORT_SHUFFLE_MANAGER_NAME, sparkConf, true); | |||
sparkConf.set(RssSparkConfig.RSS_ENABLED.key(), "false"); | |||
sparkConf.set("spark.shuffle.manager", "sort"); | |||
sparkConf.set("spark.shuffle.service.enabled", "true"); |
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.
If the user doesn't enable external shuffle, this config option shouldn't be true
.
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.
But if it is false
, sort
shuffle manager cannot work
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.
Sort shuffle manager can work without external shuffle.
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?
Fix fallback to sort fail issue
Why are the changes needed?
Support fallback to sort shuffle service.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Existing UTs.