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

flush codec after decode #81

Merged

Conversation

colinsurprenant
Copy link
Contributor

This PR depends on logstash-plugins/logstash-codec-json_lines#35

The http input only accepts self-contained payload for each request and multipart is not supported. Because of this, we should always flush the codec after each request & codec decode calls to avoid situations where a delimited content missing a final delimiter, for example, would linger in the codec.

This is similar in nature to the discussion in elastic/logstash#8830 (comment)

Spec added to test this condition. Otherwise codec support is also well tested in the current spec.

@@ -145,6 +146,20 @@ def do_post
end
end

context "with json_lines codec without final delimiter" do
subject { LogStash::Inputs::Http.new("port" => port, "codec" => "json_lines") }
Copy link
Member

Choose a reason for hiding this comment

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

can't we test this with line codec so we don't have to rely on a new feature of json_lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I guess so.

@colinsurprenant
Copy link
Contributor Author

@jsvd as you suggested I switched to the line codec, good suggestion.

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 pending on green (other than known build timeouts)

@colinsurprenant
Copy link
Contributor Author

restarted build - seems like the build issues are now solved.

@colinsurprenant
Copy link
Contributor Author

Green. Will squash, bump version, merge and publish.

remove local tweak to gemfile

use line codec instead of json_lines
@colinsurprenant
Copy link
Contributor Author

actually - I will create another PR for the version bump since it will also reference #80.

@colinsurprenant colinsurprenant merged commit 51177d6 into logstash-plugins:master Mar 22, 2018
@colinsurprenant
Copy link
Contributor Author

version bump PR in #82

@colinsurprenant
Copy link
Contributor Author

This has been released in v3.0.9

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.

3 participants