[SPARK-27428][CORE][TEST] Increase receive buffer size used in StatsdSinkSuite #29819
Conversation
…SinkSuite. The socket receive buffer size set in this test was too small for the StatsdSinkSuite tests to run reliably on some systems. For a test in this suite to run reliably the buffer needs to be large enough to hold all the data in the packets being sent in a test along with any additional kernel or protocol overhead. The amount of kernel overhead per packet can vary from system to system but is typically far higher than the protocol overhead. If the receive buffer is too small and fills up then packets are silently dropped. This leads to the test failing with a timeout. If the socket defaults to a larger receive buffer (normally true) then we should keep that size. As well as increasing the minimum buffer size I've also decoupled the datagram packet buffer size from the receive buffer size. The receive buffer should in general be far larger to account for the fact that multiple packets might be buffered, as well as the aforementioned overhead. Any truncated data in individual packets will be picked up by the tests.
|
cc @jerryshao FYI |
|
ok to test |
|
Test build #128991 has finished for PR 29819 at commit
|
dongjoon-hyun
changed the title
| // This value was determined experimentally and should be | ||
| // increased if timeouts are seen. | ||
| private val socketMinRecvBufferSize = 16384 // bytes | ||
| private val socketTimeout = 30000 // milliseconds |
There was a problem hiding this comment.
Please move this to the original position to reduce the unnecessary diff and don't add space between 30000 and // milliseconds.
There was a problem hiding this comment.
@mundaym . Why don't we simply use new value? Then, this PR can be one-liner.
- private val socketBufferSize = 8192
+ private val socketBufferSize = 16384
|
Thanks for the review @dongjoon-hyun.
Could do. It's seems to me though that having the values the same misleadingly implies that the socket receive buffer should be the same size as the UDP payload, when in fact the socket receive buffer needs to be a lot bigger (multiple packets + kernel overhead). The more I think about though it the more I'm inclined to say we should just leave the size of the socket receive buffer alone (i.e. delete the |
|
@mundaym What is the purpose of this contribution? Technically, this PR looks worth for a testing on specific machine types like IBM. Given that, the change looks a little too much. If we have one-liner, I believe we can cherry-pick to all live branches. Otherwise, maybe, this PR is limited to |
|
This is a small change to a test suite, to just increase a buffer size. I can't see that it's controversial or that a few more or less lines matters. Where are you getting that it has to be one line to backport?? |
|
No, it's not a rule~ I wrote like that because I would do if I merge this PR. Feel free to proceed in your way, @srowen . I'll leave this to you. |
|
Jenkins retest this please |
|
@mundaym are you looking for this to go into 3.0.x as well? |
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Test build #129381 has finished for PR 29819 at commit
|
|
Merged to master/3.0 |
…SinkSuite ### What changes were proposed in this pull request? Increase size of socket receive buffer in these tests. ### Why are the changes needed? The socket receive buffer size set in this test was too small for the StatsdSinkSuite tests to run reliably on some systems. For a test in this suite to run reliably the buffer needs to be large enough to hold all the data in the packets being sent in a test along with any additional kernel or protocol overhead. The amount of kernel overhead per packet can vary from system to system but is typically far higher than the protocol overhead. If the receive buffer is too small and fills up then packets are silently dropped. This leads to the test failing with a timeout. If the socket defaults to a larger receive buffer (normally true) then we should keep that size. As well as increasing the minimum buffer size I've also decoupled the datagram packet buffer size from the receive buffer size. The receive buffer should in general be far larger to account for the fact that multiple packets might be buffered, as well as the aforementioned overhead. Any truncated data in individual packets will be picked up by the tests. ### Does this PR introduce _any_ user-facing change? No, this only affects the tests. ### How was this patch tested? Existing tests on IBM Z and x86. Closes #29819 from mundaym/fix-statsd. Authored-by: Michael Munday <mike.munday@ibm.com> Signed-off-by: Sean Owen <srowen@gmail.com> (cherry picked from commit b5e4b8c) Signed-off-by: Sean Owen <srowen@gmail.com>
|
Thanks for reviewing this.
Happy for this to be backported (looks like already done). |
…SinkSuite ### What changes were proposed in this pull request? Increase size of socket receive buffer in these tests. ### Why are the changes needed? The socket receive buffer size set in this test was too small for the StatsdSinkSuite tests to run reliably on some systems. For a test in this suite to run reliably the buffer needs to be large enough to hold all the data in the packets being sent in a test along with any additional kernel or protocol overhead. The amount of kernel overhead per packet can vary from system to system but is typically far higher than the protocol overhead. If the receive buffer is too small and fills up then packets are silently dropped. This leads to the test failing with a timeout. If the socket defaults to a larger receive buffer (normally true) then we should keep that size. As well as increasing the minimum buffer size I've also decoupled the datagram packet buffer size from the receive buffer size. The receive buffer should in general be far larger to account for the fact that multiple packets might be buffered, as well as the aforementioned overhead. Any truncated data in individual packets will be picked up by the tests. ### Does this PR introduce _any_ user-facing change? No, this only affects the tests. ### How was this patch tested? Existing tests on IBM Z and x86. Closes apache#29819 from mundaym/fix-statsd. Authored-by: Michael Munday <mike.munday@ibm.com> Signed-off-by: Sean Owen <srowen@gmail.com> (cherry picked from commit b5e4b8c) Signed-off-by: Sean Owen <srowen@gmail.com>
What changes were proposed in this pull request?
Increase size of socket receive buffer in these tests.
Why are the changes needed?
The socket receive buffer size set in this test was too small for
the StatsdSinkSuite tests to run reliably on some systems. For a
test in this suite to run reliably the buffer needs to be large
enough to hold all the data in the packets being sent in a test
along with any additional kernel or protocol overhead. The amount
of kernel overhead per packet can vary from system to system but is
typically far higher than the protocol overhead.
If the receive buffer is too small and fills up then packets are
silently dropped. This leads to the test failing with a timeout.
If the socket defaults to a larger receive buffer (normally true)
then we should keep that size.
As well as increasing the minimum buffer size I've also decoupled
the datagram packet buffer size from the receive buffer size. The
receive buffer should in general be far larger to account for the
fact that multiple packets might be buffered, as well as the
aforementioned overhead. Any truncated data in individual packets
will be picked up by the tests.
Does this PR introduce any user-facing change?
No, this only affects the tests.
How was this patch tested?
Existing tests on IBM Z and x86.