Use concurrency :shared and sync codecs to optimize performance
#46
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5 tasks
lib/logstash/outputs/file.rb
Outdated
|
|
||
| private | ||
| def write_event(event, data) | ||
| def write_event(event) |
There was a problem hiding this comment.
This method should be renamed
|
my first results: With logstash 5.0.0-alpha6: Logstash master + logstash-output-file w/ this PR: I've tried this again:
|
|
Another error which |
|
@jsvd the error your reporting is strange to say the least, the first part of the message originate from the file output but the remaining part are from puma. |
|
@ph I just wanted to post the whole error stack, but the puma one is happening frequently on logstash exit, I'm going to open an issue |
|
@jsvd I am gonna take a look, we rewrote the puma integration not too long ago, I think we might be missing a rescue on close. |
|
I created an issue for the puma errors in elastic/logstash#5822 |
andrewvc
force-pushed
the
concurrent-and-sync
branch
2 times, most recently
from
6ab2a13 to
eece460
Compare
| describe "ship lots of events to a file gzipped" do | ||
| Stud::Temporary.file('logstash-spec-output-file') do |tmp_file| | ||
| event_count = 10000 + rand(500) | ||
| event_count = 100000 + rand(500) |
There was a problem hiding this comment.
I did catch some locking bugs here by upping this count to be a longer test.
|
@jsvd I've fixed all the issues here as well as made the locking a bit more coarse. I was also able to simplify the code around a simple |
andrewvc
changed the title
concurrency :shared and sync codecs to optimize performanceconcurrency :shared and sync codecs to optimize performance
andrewvc
force-pushed
the
concurrent-and-sync
branch
from
eece460 to
6c96b1d
Compare
As part of this refactor the `message_format` option had to be (finally)
obsoleted.It was previously deprecated for quite some time.
This provides a nice boost:
Before:
```
time bin/logstash -e "input { generator { count => 3000000} } filter { } output { file { path => '/tmp/newfileout'} }"
Settings: Default pipeline workers: 8
Pipeline main started
Pipeline main has been shutdown
stopping pipeline {:id=>"main"}
139.95 real 223.61 user 28.93 sys
```
After
```
rm /tmp/newfileout; time bin/logstash -e "input { generator { count => 3000000} } filter { } output { file { codec => json_lines path => '/tmp/newfileout'} }" ; ls -lh /tmp/newfileout
Settings: Default pipeline workers: 8
Pipeline main started
Pipeline main has been shutdown
stopping pipeline {:id=>"main"}
56.12 real 192.99 user 17.38 sys
```
andrewvc
force-pushed
the
concurrent-and-sync
branch
from
6c96b1d to
ee244b7
Compare
|
@jsvd I've bumped the plugin API so it will require LS 5.0+ to properly handle the obsolescence. |
|
Tests pass, manual tests also good, nice work |
|
Thanks @jsvd ! |
|
Andrew Cholakian merged this into the following branches!
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a WIP. This commit has added locking, but I may have missed some locks in a few spots, so it needs another pass before its really ready.
This provides a nice boost:
Before:
After