search

logstash-plugins / logstash-input-gelf

Apache License 2.0
20 stars 39 forks source link

TCP support based on Ifedix #59

Closed Hermain closed 6 years ago

Hermain commented 6 years ago

Based on https://github.com/logstash-plugins/logstash-input-gelf/pull/57 I resolved some of the comments.

Tested it with the following config: logstash -e 'input { gelf { use_tcp => true use_udp => true } } output { stdout{ } }' while streaming tcp and udp at the same time. It works

fixes https://github.com/logstash-plugins/logstash-input-gelf/issues/6

paladox commented 6 years ago

Does switching off udp and keeping tcp on work?

And same for the other way around?

karmi commented 6 years ago

Hi @Hermain, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

Hermain commented 6 years ago

@karmi I added my work email address. I hope you can verify it now

Hermain commented 6 years ago

I tested it with 3 configs and all work as expected: input { gelf { use_tcp => true use_udp => true } } input { gelf { use_tcp => true use_udp => false } } input { gelf { use_tcp => false use_udp => true } }

paladox commented 6 years ago

I think the tests need updating too, failing here https://travis-ci.org/logstash-plugins/logstash-input-gelf/jobs/328085227

paladox commented 6 years ago

You will want to update https://github.com/logstash-plugins/logstash-input-gelf/blob/master/spec/inputs/gelf_spec.rb to use port_udp as it was renamed to that and port_tcp added.

Hermain commented 6 years ago

---> rewrote the stop method to handle both threads. ----> Reintroduced udp as default

mariusmitrofan commented 6 years ago

Hey guys, I can't wait to get the ball running on this. When do you guys think we can release this?

paladox commented 6 years ago

Test failures https://travis-ci.org/logstash-plugins/logstash-input-gelf/jobs/328495467

Hermain commented 6 years ago

As far as I can tell the master branch suffers from this test failure too so it shouldn't hold up this PR: https://travis-ci.org/logstash-plugins/logstash-input-gelf/jobs/321772980

paladox commented 6 years ago

Oh i see.

paladox commented 6 years ago

@Hermain hi, but the failures look different in master, see https://travis-ci.org/logstash-plugins/logstash-input-gelf/builds/321772979?utm_source=github_status&utm_medium=notification which is from https://github.com/logstash-plugins/logstash-input-gelf/commit/604c42cb69e1c53467c2f0a22a69429c88b95a46

https://travis-ci.org/logstash-plugins/logstash-input-gelf/jobs/328495467

jsvd commented 6 years ago

@Hermain I created a commit that addresses a couple of things on the testing and start/stopping side, and seems to make all tests pass: https://github.com/logstash-plugins/logstash-input-gelf/pull/60

you can apply my commit to your PR with:

curl https://github.com/jsvd/logstash-input-gelf/commit/ffd34dece03d52b250ef879831f23850540512b8.patch | git am

After this I think we only need a few touches on the docs/index.asciidoc since that is the source of truth for documentation.

Hermain commented 6 years ago

Lets get this merged :)

jsvd commented 6 years ago

I think we're done here. I'll merge this into master by squashing commits so that both @Hermain and @iFedix retain attribution.

jsvd commented 6 years ago

I merged the following commits:

5362970 - Added TCP Support - iFedix 6b254c0 - Rework of iFedix initial patch for TCP support - Hermain 1f20593 - minor documentation cleanups and test fixes - Joao Duarte 5ef40b2 - bump to 3.1.0 - Joao Duarte

It was a pleasure @iFedix @Hermain (and @paladox)! Thanks for the contribution and patience throughout the review.

Version 3.1.0 with your changes has been published to rubygems