minor: Split CometShuffleExternalSorter into sync/async implementations#3192
minor: Split CometShuffleExternalSorter into sync/async implementations#3192mbutrovich merged 1 commit intoapache:mainfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3192 +/- ##
============================================
+ Coverage 56.12% 59.92% +3.79%
- Complexity 976 1428 +452
============================================
Files 119 170 +51
Lines 11743 15684 +3941
Branches 2251 2595 +344
============================================
+ Hits 6591 9398 +2807
- Misses 4012 4971 +959
- Partials 1140 1315 +175 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
andygrove
requested review from
comphead,
mbutrovich,
parthchandra and
wForget
|
Do we have any evidence to support performance benefits for the async implementation? Async only makes sense to me if we believe these workers have other meaningful scheduled work to do. Otherwise they should probably be blocking and just yield. This actually makes me wonder more how to instrument/measure the state of worker pools at various states of execution. I feel like we're making a lot of assumptions (e.g., "async good") that aren't necessary supported by data yet. |
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
I think this refactor looks good. We can take my existential question elsewhere. Thanks @andygrove!
3 participants
Which issue does this PR close?
Closes #.
Rationale for this change
CometShuffleExternalSortercontains both sync and async code. Splitting into two separate implementations will make it easier to clean up the code in both paths.What changes are included in this PR?
No functional changes.
There is some code duplication between the two implementations. I will address this in the next PR.
How are these changes tested?