Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix drop events between two logstash instances #7

Closed
wants to merge 8 commits into from
Closed

Fix drop events between two logstash instances #7

wants to merge 8 commits into from

Conversation

ph
Copy link
Contributor

@ph ph commented Aug 11, 2015

This PR introduce the bulk send feature of the ruby client and add a
local buffer inside this plugin. Theses changes make the plugin closer
to the LSF behavior.

ph added 2 commits August 10, 2015 20:32
@ph
Changing the default value for the window_size to have the same values
c0fd46c 
as the LSF's spoolsize
@ph
Fix drop events between two logstash instances
7e7b4a1 
This PR introduce the `bulk` send feature of the ruby client and add a
local buffer inside this plugin. Theses changes make the plugin closer
to the LSF behavior.
@ph
Copy link
Contributor Author

ph commented Aug 11, 2015

@jsvd Can you test this? you need latest version of elastic/ruby-lumberjack#9

@ph ph mentioned this pull request Aug 11, 2015
Closed
jordansissel
jordansissel reviewed Aug 12, 2015
View reviewed changes
spec/outputs/lumberjack_spec.rb
"hosts" => [host],
"port" => port,
"ssl_certificate" => certificate_file_crt,
"flush_size" => 10#batch_size # flush at the end of the payload
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

10? why not batch_size ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually test with static variable before adding random values this is left over.

@jordansissel
Copy link
Contributor

@ph for clarification, this PR requires elastic/ruby-lumberjack#9 be released first, right?

@ph
Copy link
Contributor Author

ph commented Aug 13, 2015

@jordansissel yes, elastic/ruby-lumberjack#9 add support for bulk send which this PR need

@ph
Copy link
Contributor Author

ph commented Aug 13, 2015

code review done

@ph
Copy link
Contributor Author

ph commented Aug 17, 2015

@jordansissel Can I get a LGTM for this one? I'll squash after.

@ph ph assigned jordansissel and unassigned jsvd Aug 17, 2015
@jordansissel
Copy link
Contributor

@ph will review shortly <3

@jordansissel
Copy link
Contributor

Code + Test code looks good, will test shortly.

@jordansissel
Copy link
Contributor

Can't build the gem:

ERROR:  While executing gem ... (Gem::InvalidSpecificationException)
    duplicate dependency on stud (>= 0, development), (>= 0) use:
    add_runtime_dependency 'stud', '>= 0', '>= 0'

jordansissel
jordansissel reviewed Aug 17, 2015
View reviewed changes
logstash-output-lumberjack.gemspec
@@ -24,7 +24,12 @@ Gem::Specification.new do |s|
s.add_runtime_dependency "logstash-core", '>= 1.4.0', '< 2.0.0'

s.add_runtime_dependency 'jls-lumberjack', ['>=0.0.23']
s.add_runtime_dependency "stud"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rubygems doesn't allow both runtime and dev dependency with the same lib?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish bundle install would also yield theses errors since he is using the gemspec to install the depedencies, TIL to always test with gem build my changes to a gemspec.
bart-simpson-generator

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the PR.

  Successfully built RubyGem
  Name: logstash-output-lumberjack
  Version: 1.0.1
  File: logstash-output-lumberjack-1.0.1.gem

@purbon purbon mentioned this pull request Aug 18, 2015
Closed
30 tasks
@jordansissel
Copy link
Contributor

Confirmed tests passing for me

@jordansissel
Copy link
Contributor

Tested perf is 2x (eyeballing it with pv -War) with this patch (3k/sec -> 6k/sec). We can probably improve further but this is a lovely start :)

@elasticsearch-bot
Copy link

Merged sucessfully into master!

@ph ph closed this in d32fb00 Aug 19, 2015
ph added a commit that referenced this pull request Aug 19, 2015
@ph @jordansissel
Fix drop events between two logstash instances
4ab5b75 
This PR introduce the `bulk` send feature of the ruby client and add a
local buffer inside this plugin. Theses changes make the plugin closer
to the LSF behavior.

Fixes #7
ph added a commit that referenced this pull request Aug 19, 2015
@ph @jordansissel
Make sure flush_size has a higher priority in the configuration
a3ad4ca 
Fixes #7
ph added a commit that referenced this pull request Aug 19, 2015
@ph @jordansissel
should be a dev dependencies
c228dae 
Fixes #7
ph added a commit that referenced this pull request Aug 19, 2015
@ph @jordansissel
Code review
133d6a6 
Fixes #7
ph added a commit that referenced this pull request Aug 19, 2015
@ph @jordansissel
input -> output
d37cd3d 
Fixes #7
ph added a commit that referenced this pull request Aug 19, 2015
@ph @jordansissel
Make sure stud is only a runtime dependencies.
f0eb976 
Fixes #7
ph added a commit that referenced this pull request Aug 19, 2015
@ph @jordansissel
Force dependencies on jls-lumberjack .24
e60f43d 
Fixes #7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jordansissel jordansissel

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ph @jordansissel @elasticsearch-bot @jsvd