WIP Use java DatagramSocket directly #38
Conversation
|
I haven’t tested or benchmarked this PR, but it might be worthwhile to use Netty instead of the Java socket API. Other plugins like logstash-input-tcp already do. (Also, this would be so much simpler once the Logstash Java API is available!) |
|
Netty would be worth exploring I agree, but I think it makes sense to solve
this memory issue in a minimal way for the time being.
I agree Netty makes more sense as a pure java plugin.
…On Fri, Apr 20, 2018, 5:47 PM Mark Janssen ***@***.***> wrote:
I haven’t tested or benchmarked this PR, but it might be worthwhile to use
Netty <https://netty.io> instead of the Java socket API. Other plugins
like logstash-input-tcp
<https://github.com/logstash-plugins/logstash-input-tcp> already do.
(Also, this would be so much simpler once the Logstash Java API is
available!)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#38 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAIBY6dGMHvDtTM5PE_O64vet-Ick6PSks5tqmV3gaJpZM4TeKGz>
.
|
|
You need to assign to rcvbuf |
|
Also, it's not clear to me that for UDP Netty would be much faster than
blocking IO. At any rate, it's a question we should ask when we port to
Java.
…On Fri, Apr 20, 2018, 6:30 PM Andrew Cholakian ***@***.***> wrote:
Netty would be worth exploring I agree, but I think it makes sense to
solve this memory issue in a minimal way for the time being.
I agree Netty makes more sense as a pure java plugin.
On Fri, Apr 20, 2018, 5:47 PM Mark Janssen ***@***.***>
wrote:
> I haven’t tested or benchmarked this PR, but it might be worthwhile to
> use Netty <https://netty.io> instead of the Java socket API. Other
> plugins like logstash-input-tcp
> <https://github.com/logstash-plugins/logstash-input-tcp> already do.
>
> (Also, this would be so much simpler once the Logstash Java API is
> available!)
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#38 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAIBY6dGMHvDtTM5PE_O64vet-Ick6PSks5tqmV3gaJpZM4TeKGz>
> .
>
|
|
@IrlJidel good catch on |
|
Let me know when you think it's OK, so I can run perf tests using this patch and see the results before releasing it. |
| @logger.warn("Unable to set receive_buffer_bytes to desired size. Requested #{@receive_buffer_bytes} but obtained #{rcvbuf} bytes.") | ||
| @udp.setReceiveBufferSize(@receive_buffer_bytes) | ||
|
|
||
| actual_receive_buffer_bytes = @udp.getReceiveBufferBytes |
There was a problem hiding this comment.
Should be @udp.getReceiveBufferSize
|
|
||
| @udp.bind(@host, @port) | ||
| @logger.info("UDP listener started", :address => "#{@host}:#{@port}", :receive_buffer_bytes => "#{rcvbuf}", :queue_size => "#{@queue_size}") | ||
| @logger.info("UDP listener started", :address => "#{@host}:#{@port}", :receive_buffer_bytes => "#{@receive_buffer_bytes}", :queue_size => "#{@queue_size}") |
There was a problem hiding this comment.
To keep to previous behavior we should be logging using actual_receive_buffer_bytes rather than @receive_buffer_bytes
|
I'd like to attempt a fix of the issue with JRuby instead of going directly to a rewrite of the plugin. For reference, a workaround was proposed by @original-brownbear here. That said I'm +1 on exploring a more native implementation, just not for the reason of fixing the memory usage issue. |
|
jruby RubyString doesn't have a .b method |
|
I agree that JRuby should be fixed.
Joao, why not do both? Using the java classes directly is going to be more
efficient. This refactor is relatively small. If you think it's not
worthwhile however I'm glad to abandon it.
…On Mon, Apr 23, 2018, 6:07 AM João Duarte ***@***.***> wrote:
I'd like to attempt a fix of the issue with JRuby instead of going
directly to a rewrite of the plugin. For reference, a workaround was
proposed by @original-brownbear <https://github.com/original-brownbear>
here
<elastic/logstash#9346 (comment)>.
That said I'm +1 on exploring a more native implementation, just not for
the reason of fixing the memory usage issue.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#38 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAIBY7ChTwPmrWzGCYi0cm7HR7UzXs0Tks5trbXOgaJpZM4TeKGz>
.
|
|
I put the perf test results in elastic/logstash#9346 (comment) but might make more sense to have them here: tested on LS Tests were run for 15minutes with 300 byte message.
Couldn't test using @original-brownbear suggestion to use |
I seem to have explained myself badly for you arrive at this statement. I think it's worth exploring a more native What I want to alert to is that it's different to review and accept a PR that rewrites the critical path of the plugin vs one that fixes an issue by changing a small part (how the data read from the buffer is handled). To sum up, for this PR to be the solution for the fix, it needs to go to extra lengths to provide enough confidence that current use cases are not worse overall (if it improves some or even all, great!). |
|
@jsvd those are all valid concerns. I have a lot of irons in the fire and likely won't be able to address things in that detail given time concerns. I'm going to leave this PR open, but I won't do further work on it. |
|
Just for reference, I tried running this on logstash 6.2.4 and got an exception when setting the host field in the event: |
|
We should, by the way, aim for a minimal refactor, re-initializing the string somehow to free that memory. |
|
Also, for reference, I created an issue in JRuby about this: jruby/jruby#5148 |
|
We could also simply print a warning log message stating that using the default buffer size is bad for peformance. Or, if we'd want to get better out-of-the-box performance, decrease the default buffer size to 1472 and add a warning message that the buffer size should be set explicitly to the maximum message length that the user expects. |
|
I created a PR that works around the memory issue #39 |
Attempts to address elastic/logstash#9346 by just using the Java UDP API directly. The issue there is that JRuby's implementation uses too much memory, the underlying ruby strings returned from the UDP socket use up as much memory as the UDP buffer size, regardless of how big the actual packet is.
I would like to benchmark this and make one more pass before this is ready for review.