-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-3133] embed small object in broadcast to avoid RPC #2681
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
|
QA tests have started for PR 2681 at commit
|
|
QA tests have finished for PR 2681 at commit
|
|
Test FAILed. |
|
QA tests have started for PR 2681 at commit
|
|
QA tests have finished for PR 2681 at commit
|
|
Test PASSed. |
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.
when will this ever happen?
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.
In the case of having multiple sparkenv in different threads, then this will happen, also blockmanager will return an invalid results.
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.
can u add some inline comment explaining this? thanks.
|
Should we change the default EMBEDDED_SIZE to 8k? maybe we could do it later. |
|
QA tests have started for PR 2681 at commit
|
|
QA tests have finished for PR 2681 at commit
|
|
Test PASSed. |
|
QA tests have started for PR 2681 at commit
|
|
QA tests have finished for PR 2681 at commit
|
|
Sometimes hit this bug during pyspark testing The test is really simple: The serialized PythonRDD is about 4.1k, so I would like to increase the EMBED_SIZE to 8k, then most of the simple PythonRDD will be embedded, to make tests stable. |
|
Test FAILed. |
|
@davies: that exception that you hit is very helpful; it looks like you've been able to reproduce SPARK-3958 in local tests. That JIRA describes some reports of cases where we've seen TorrentBroadcast throw similar errors, but I've had a really hard time debugging that issue because I've had trouble reliably reproducing this bug. Why do you think |
|
This error was not happened in tests of this PR, it happened in tests of our product, which have similar pattern as streaming, the job was submitted via py4j. The PR also check the number of blocks in readBlocks(), will throw a meaningful exception in case of fail to get cached object in local mode. TorrentBroadcast is so complicated (including several RPC) that it's not as stable as HTTPBroadcast or w/o broadcast (we had saw some cases reported by users in maillist), the motivation of this PR is to remove the complicity for most cases (serialized task is small), then it will be more stable. |
|
QA tests have started for PR 2681 at commit
|
|
QA tests have finished for PR 2681 at commit
|
|
Based on some offline discussions, we think that we may have identified the cause of the Snappy issues that we've seen with TorrentBroadcast. I'm going to merge this PR and then work on some refactoring that may fix the underlying bug (or at least add additional log information to aid in debugging). I also have some thoughts on how to improve the test coverage of our broadcast implementation, but I'l address this in my PR. Thanks @davies! |
|
Actually, I changed my mind: I'm going to hold off on merging this because I don't want to backport the embedding of small objects into |
|
I hope that we can have this in 1.1, some people see regression in 1.1 because of TorrentBroadcast, this patch will help for those. |
Conflicts: core/src/main/scala/org/apache/spark/broadcast/TorrentBroadcast.scala
|
Test build #22151 timed out for PR 2681 at commit |
|
Test FAILed. |
|
Even if we merge #2933, I still would like to have this, because people could use broadcast for small dataset (such as in MLlib), this patch can improve these cases. |
|
Test build #419 has started for PR 2681 at commit
|
|
Test build #419 has finished for PR 2681 at commit
|
|
Test build #429 has started for PR 2681 at commit
|
|
Test build #429 timed out for PR 2681 at commit |
|
Test build #481 has started for PR 2681 at commit
|
|
Test build #481 has finished for PR 2681 at commit
|
|
Test build #483 has started for PR 2681 at commit
|
|
Test build #483 has finished for PR 2681 at commit
|
Conflicts: core/src/main/scala/org/apache/spark/broadcast/TorrentBroadcast.scala
|
Test build #22367 has started for PR 2681 at commit
|
|
Test FAILed. |
|
Test build #487 has started for PR 2681 at commit
|
|
Test build #22367 has finished for PR 2681 at commit
|
|
Test PASSed. |
|
Test build #487 has finished for PR 2681 at commit
|
|
Test build #500 has started for PR 2681 at commit
|
|
Test build #500 has finished for PR 2681 at commit
|
|
ping dashboard |
|
@JoshRosen @pwendell @rxin This is an optimization for TorrentBroadcast (avoid all RPC for small objects), which can reduce latency for streaming (not benchmarked). Do you mind to merge this into 1.2? |
|
I've pushed a commit to |
|
Test build #23808 has started for PR 2681 at commit
|
|
Test build #23808 has finished for PR 2681 at commit
|
|
Test PASSed. |
|
I'd like to close this PR now, will re-open it once needed. |
For most of tasks, the serialized data will small, such as less than 8k, we can avoid the RPC at all if the data was embedded in the Broadcast object it self.
With this patch, The size of task will be similar to that before we use broadcast for them, no RPC (but still cached, only one deserialization per executor)
It will increase the bandwidth during schedule tasks, for example, if we schedule 10k tasks per seconds, then it will need 40M Bytes more bandwidth for the embedded object in the worst cases.