Bugfix for uninterrupted receive loop #18

breml · 2015-12-02T13:11:47Z

Replace the uninterrupted receive loop (non-blocking) with
a blocking loop with a default timeout of 1000ms.

Restores compatibility with zeromq output plugin for util/zeromq.rb as well, if logstash-plugins/logstash-output-zeromq#13 is merged.

Closes #17
May also be related to #5

Adjust implementation of util/zeromq.rb to zeromq output. logstash-plugins/logstash-output-zeromq#13

Replace the uninterrupted receive loop (non-blocking) with a blocking loop with a default timeout of 1000ms.

elasticsearch-release · 2015-12-02T13:12:10Z

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

breml · 2015-12-11T13:31:44Z

lib/logstash/util/zeromq.rb

@@ -20,8 +20,8 @@ def setup(socket, address)
    @logger.info("0mq: #{server? ? 'connected' : 'bound'}", :address => address)
  end

-  def error_check(rc, doing)
-    unless ZMQ::Util.resultcode_ok?(rc)
+  def error_check(rc, doing, eagain_not_error = false)


Because function signature of error_check is changed and util/zeromq.rb is present in zeromq input and output plugin, this PR has a dependecy to logstash-plugins/logstash-output-zeromq/pull/13.

breml · 2015-12-18T09:29:14Z

Could please someone give me some feedback?

erikthorselius · 2015-12-26T20:26:36Z

This would be really good if any maintainer could have a look on this.

nukemberg · 2015-12-27T06:57:46Z

OK, had a look. The patch looks straight forward. I do need to run some tests so it will take a few days to close this. Thanks!

erikthorselius · 2016-01-06T07:19:54Z

I'm really interested in this fix. I'm having two zeromq inputs taking up half of the server cpu capacity. So if there is any thing I can do to help, feel free to delegate.

breml · 2016-01-06T10:24:53Z

@erikthorselius Have you tried this PR in your setup? Did it solve your problem?

erikthorselius · 2016-01-06T19:06:09Z

Looks good! I'm new to logstash but here is how I tested. My server is running coreos so I built a docker container.

My dockerfile:

FROM logstash
COPY logstash-input-zeromq-2.0.2.gem /tmp/
RUN /opt/logstash/bin/plugin uninstall logstash-input-zeromq
RUN /opt/logstash/bin/plugin install /tmp/logstash-input-zeromq-2.0.2.gem
CMD ["logstash", "agent"]

gem build logstash-input-zeromq.gemspec
docker build -t logstash-with-zeromq-patch .
docker save -o logstash-with-zeromq-patch logstash-with-zeromq-patch
scp logstash-with-zeromq-patch server:

and on my server

docker load -i logstash-with-zeromq-patch
docker run --rm --name logstash -p 2120:2120 -v /home/logstash/conf:/data logstash-with-zeromq-patch logstash -f /data/logstash.conf

Same process is now down to 1.8% instead of 200%

input {
  zeromq {
    topology => "pushpull"
    tags => ["pushpull"]
  }
  zeromq {
    mode => "client"
    address => ["tcp://192.168.1.3:7555"]
    topology => "pubsub"
    topic => "sensor"
    tags => ["pubsub"]
  }
}
filter {
  date {
    match => ["unix_timestamp", "UNIX"]
  }
}
output {
  elasticsearch {
    hosts => ["192.168.1.3:9200"]
  }
}

jordansissel · 2016-01-06T22:23:09Z

lib/logstash/inputs/zeromq.rb

+  #
+  # defaults to: `sockopt => ["ZMQ::RCVTIMEO", "1000"]`, which has the effect of "interrupting"
+  # the recv operation at least once every second to allow for properly shutdown handling.
+  config :sockopt, :validate => :hash, :default => [ "ZMQ::RCVTIMEO", "1000" ]


This should probably be :default => { "ZMQ::RCVTIMEO" => "1000" } right?

I agree, it should be a hash. I just followed the existing example, which provides an array with an even count of elements, which gets converted to a hash in https://github.com/elastic/logstash/blob/master/logstash-core/lib/logstash/config/mixin.rb#L381 (lines 381 - 398). I update the PR, including the existing comment/example.

jordansissel · 2016-01-06T22:24:19Z

Given @erikthorselius's success report, I'm OK merging this. Code LGTM except for my one comment about the hash default value (should use hash syntax, not array).

@breml Can you address my comment? I'm happy to merge once done. :)

erikthorselius · 2016-01-07T07:38:56Z

It fixed my cpu problem but it seems that I don't receive any messages.

I'm at work right now but I can try to debug when I'm home.

breml · 2016-01-07T07:49:54Z

@erikthorselius Please see my comment regarding lib/logstash/util/zeromq.rb. This file was shared in Logstash prior to version 1.5. Now both plugins, logstash-input-zeromq and logstash-output-zeromq do provide this file. Therefore it is important to update this file in both plugins at the same time because otherwise it could happen that the logstash-input-zeromq plugin is using the wrong version of this file. (see my reference to logstash-plugins/logstash-output-zeromq#13 above)

@jordansissel this is quiet an annoying issue. I suggest to either really duplicate the code by moving lib/logstash/util/zeromq.rb to lib/logstash/inputrespectively lib/logstash/output or by putting lib/logstash/util/zeromq.rb into a separate gem file, which is really shared and therefore present only once.

erikthorselius · 2016-01-07T08:11:58Z

Maybe a stupid question but how do I update that? Would it work to uninstall logstash-output-zeromq?

breml · 2016-01-07T09:23:06Z

@erikthorselius Uninstalling the logstash-output-zeromq plugin may work. If it doesn't, I recommend to rename ./vendor/bundle/jruby/1.9/gems/logstash-output-zeromq-2.0.2/lib/logstash/util/zeromq.rb for example to zeromq.rb.bak. This will prevent ruby from loading this file which will result in ruby loading the correct one from the logstash-input-zeromq plugin.

erikthorselius · 2016-01-31T17:36:14Z

I'm so sorry forgetting this. I blame work.

I have tested this again and it works perfect! The cpu goes from 200% to 0.3 % and i still receive logs.

nukemberg · 2016-02-01T07:18:47Z

i'm going to start working on refactoring common zeromq code to a new gem, this should ease maintenance of logstash zeromq plugins. i'll update soon.

suyograo · 2016-02-01T07:55:51Z

@breml Thanks for this. Could you please bump the version in the gemspec and add a changelog? I can merge this, then and publish a new version

breml · 2016-02-02T10:38:26Z

@suyograo Sure I would like to update gemspec and changelog, but this change should not be merged without either logstash-plugins/logstash-output-zeromq#13 or the refactoring mentioned by @avishai-ish-shalom, because of https://github.com/logstash-plugins/logstash-input-zeromq/pull/18/files#r47354652.

nukemberg · 2016-03-17T06:43:48Z

This PR is on hold until logstash-mixin-zeromq is integrated. cc @breml

Lucas Bremgartner added 2 commits December 2, 2015 13:59

Move EAGAIN is not error to util/zeromq.rb

668cfe2

Adjust implementation of util/zeromq.rb to zeromq output. logstash-plugins/logstash-output-zeromq#13

Bugfix for uninterrupted receive loop

676f90b

Replace the uninterrupted receive loop (non-blocking) with a blocking loop with a default timeout of 1000ms.

breml reviewed Dec 11, 2015
View reviewed changes

breml mentioned this pull request Dec 11, 2015

Added eagain_not_error and noblock config options logstash-plugins/logstash-output-zeromq#13

Open

jordansissel reviewed Jan 6, 2016
View reviewed changes

jordansissel self-assigned this Jan 6, 2016

Default for sockopt as hash, update docs

d93b0bd

breml mentioned this pull request Mar 14, 2016

Add logstash-util-zeromq to logstash-plugins elastic/logstash#4813

Open

breml mentioned this pull request Apr 4, 2016

Initial version of logstash-mixin-zeromq logstash-plugins/logstash-mixin-zeromq#1

Merged

nukemberg merged commit d93b0bd into logstash-plugins:master Apr 18, 2016

breml deleted the feature-eagain-not-error branch April 18, 2016 13:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix for uninterrupted receive loop #18

Bugfix for uninterrupted receive loop #18

breml commented Dec 2, 2015

elasticsearch-release commented Dec 2, 2015

breml Dec 11, 2015

breml commented Dec 18, 2015

erikthorselius commented Dec 26, 2015

nukemberg commented Dec 27, 2015

erikthorselius commented Jan 6, 2016

breml commented Jan 6, 2016

erikthorselius commented Jan 6, 2016

jordansissel Jan 6, 2016

breml Jan 7, 2016

jordansissel commented Jan 6, 2016

erikthorselius commented Jan 7, 2016

breml commented Jan 7, 2016

erikthorselius commented Jan 7, 2016

breml commented Jan 7, 2016

erikthorselius commented Jan 31, 2016

nukemberg commented Feb 1, 2016

suyograo commented Feb 1, 2016

breml commented Feb 2, 2016

nukemberg commented Mar 17, 2016

Bugfix for uninterrupted receive loop #18

Bugfix for uninterrupted receive loop #18

Conversation

breml commented Dec 2, 2015

elasticsearch-release commented Dec 2, 2015

breml Dec 11, 2015

Choose a reason for hiding this comment

breml commented Dec 18, 2015

erikthorselius commented Dec 26, 2015

nukemberg commented Dec 27, 2015

erikthorselius commented Jan 6, 2016

breml commented Jan 6, 2016

erikthorselius commented Jan 6, 2016

jordansissel Jan 6, 2016

Choose a reason for hiding this comment

breml Jan 7, 2016

Choose a reason for hiding this comment

jordansissel commented Jan 6, 2016

erikthorselius commented Jan 7, 2016

breml commented Jan 7, 2016

erikthorselius commented Jan 7, 2016

breml commented Jan 7, 2016

erikthorselius commented Jan 31, 2016

nukemberg commented Feb 1, 2016

suyograo commented Feb 1, 2016

breml commented Feb 2, 2016

nukemberg commented Mar 17, 2016