-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-6521][Core] Bypass unnecessary network access if block managers share an identical host #9478
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
Conversation
|
Test build #45071 has finished for PR 9478 at commit
|
|
Test build #45106 has finished for PR 9478 at commit
|
5c00a5a to
8d0a2f0
Compare
|
Test build #45117 has finished for PR 9478 at commit
|
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.
nit: each arg on its own line, with return value on same line as last arg
|
retest this please |
|
Test build #45163 has finished for PR 9478 at commit
|
|
Test build #45166 has finished for PR 9478 at commit
|
|
Test build #45170 has finished for PR 9478 at commit
|
|
@andrewor14 Could you review this and give some suggestions? |
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.
this doesn't belong here. BlockDataManager should know nothing about shuffles.
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.
Renamed getShuffleBlockData into getBlockData. Is this fix correct for your comments?
|
@maropu Thanks for taking this over. I took a pass and I believe the high level approach is correct. However, it seems that the abstractions can be simplified a little. Please also fix the style issues that I pointed out. |
|
By the way, have you done some benchmarking to measure how much this actually saves? I wonder if the gains are actually all that significant. Maybe it actually matters if we're fetching many small blocks, but even then I'm not 100% sure. |
|
@andrewor14 Thx for your reviews. |
cebc895 to
6b8f7bf
Compare
|
Test build #45591 has finished for PR 9478 at commit
|
|
@maropu any updates? Did you have a chance to do the benchmarks? |
|
@andrewor14 I tried quick benchmarks though, I saw little difference, so I'm taking various tests on it. |
6b8f7bf to
1653691
Compare
|
Test build #46739 has finished for PR 9478 at commit
|
|
Test build #46741 has finished for PR 9478 at commit
|
4ca2d72 to
ba94687
Compare
|
Test build #46794 has finished for PR 9478 at commit
|
0325d35 to
303abcd
Compare
|
@andrewor14 ISTM that this pr has a little effect on performance even in case of many partitions involved in shuffle. Test settings:
I think that this patch might avoid issues caused by netty (i.e., gc pressures and network troubles) in a corner case though, |
|
Test build #46806 has finished for PR 9478 at commit
|
|
@maropu thanks for running the benchmarks. It seems that the gains are not really significant enough to warrant all the complexity this patch adds. This is to a certain extent as expected since network is generally pretty fast, and Spark tends to bottleneck on CPU rather than I/O or network. |
|
Can you close this PR? |
|
@andrewor14 Okay and thanks. Also, can you close SPARK-6521? I left comments there. |
Refactored #5178 and added unit tests.