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

use Event#from_json if available #19

Merged
merged 2 commits into from
Feb 9, 2016
Merged

use Event#from_json if available #19

merged 2 commits into from
Feb 9, 2016

Conversation

colinsurprenant
Copy link
Contributor

relates to https://github.com/elastic/logstash/pull/4631and elastic/logstash#4595

the decode method will use Event#from_json if it is available for a significant performance improvement with the Java Event implementation.

This was referenced Feb 5, 2016
Merged
Closed
@talevy
Copy link
Contributor

talevy commented Feb 5, 2016

why do we need to preserve support for legacy? I feel like if that is necessary, then #4631 should be modified to the point that it just works

@jsvd
Copy link
Member

jsvd commented Feb 5, 2016

LGTM

@jsvd
Copy link
Member

jsvd commented Feb 5, 2016

Well it's a new api on LogStash::Event so either we force a minimum version for logstash-core or support legacy "for a while".

@guyboertje
Copy link
Contributor

+1 Good stuff.

@colinsurprenant
Copy link
Contributor Author

@talevy as @jsvd said, Java Event will go into 2.3 which is a minor version release but adding from_json is a new API so plugins depending on 2.x need to be working with both APIs.

@colinsurprenant
Copy link
Contributor Author

this makes me realize we should document that in the code.

@colinsurprenant
Copy link
Contributor Author

will wait to see what we do related to the json array case as discussed in elastic/logstash#4631

@colinsurprenant
Copy link
Contributor Author

Last commit not handles the new from_json array result.

@colinsurprenant colinsurprenant mentioned this pull request Feb 8, 2016
Merged
@colinsurprenant
Copy link
Contributor Author

thanks. fixed :P

@talevy
Copy link
Contributor

talevy commented Feb 8, 2016

can we add tests for both the new and legacy?

@colinsurprenant
Copy link
Contributor Author

@talevy it would be a good idea but not sure how exactly. the alias method happens at class load time so once in the specs that alias already exists. maybe re-alias in the specs? but this is very implementation specific. maybe move the aliasing in the register method and expose a new public method such as from_json_exposed? (instead of doing Event.respond_to?(:from_json)) to be able to mock it in specs to force choose one impl or the other?

@colinsurprenant
Copy link
Contributor Author

@talevy I just pushed changes to support testing both contexts. let me know what you think and if that makes sense I'll apply the same logic to the json codec.

@talevy
Copy link
Contributor

talevy commented Feb 8, 2016

LGTM

@colinsurprenant
use Event#from_json when possible
6186ad6 
cosmetic reindent

refactor for the array return value of from_json

wrong paramter

refactor to support testing from_json and legacy parser

refactor to support testing from_json and legacy parser - take2
@colinsurprenant
Copy link
Contributor Author

rebasing, bumping version, merging & pushing.

@colinsurprenant colinsurprenant merged commit 84c41b0 into logstash-plugins:master Feb 9, 2016
@colinsurprenant
Copy link
Contributor Author

published 2.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@colinsurprenant @talevy @jsvd @guyboertje