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.

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

add flush support #35

Merged
merged 2 commits into from
Mar 22, 2018
Merged

Conversation

colinsurprenant
Copy link
Contributor

@colinsurprenant colinsurprenant commented Mar 21, 2018

Fixes #20 and it also fixes the issue with inputs like http where each received payload will always yield one or many json lines but may not contain a delimiter on the last one. These inputs need to call flush after each request to make sure any buffers are emptied.

This codec will now be consistent with the line codec which also support the flush method in the same way.

Also, for reference, this flush "semantic" was discussed in elastic/logstash#8830

@jsvd
Copy link
Member

jsvd commented Mar 22, 2018

could this be considered a breaking change? are there other places where we've done this already?

@colinsurprenant
Copy link
Contributor Author

@jsvd this is exactly the same idea as with the line codec, any/all delimited content codec should support the flush method for situation such as logstash-plugins/logstash-input-http#81 or logstash-plugins/logstash-input-udp@ecf93e1#diff-f55c7d32f8cd66e4be73265d1b1d2342R122

In essence, this is not a breaking change but a fix to a potential data loss or corruption problem with inputs such as udp or http.

Copy link
Member

@jsvd jsvd left a comment

Choose a reason for hiding this comment

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

LGTM

@colinsurprenant
Copy link
Contributor Author

Thanks @jsvd
bumped version to 3.0.6, will merge and publish.

@colinsurprenant colinsurprenant merged commit 17fba89 into logstash-plugins:master Mar 22, 2018
@colinsurprenant colinsurprenant self-assigned this Mar 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

implement codec flush method
3 participants