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

Replace all grok :int coercions with :long coercions #9638

Merged
merged 1 commit into from
Dec 19, 2018

Conversation

webmat
Copy link
Contributor

@webmat webmat commented Dec 18, 2018

Addresses the section "Modules already merged" of #9598

Note that the strategy here is to coerce absolutely all integers with :long, even if they have no chance to overflow a signed int. Trying to keep the approach simple in the future.

The only place where it may make sense to keep :int is actually for the status codes, which stop in the 500s. The port and PIDs will overflow the signed int.

@webmat webmat self-assigned this Dec 18, 2018
@webmat webmat added in progress Pull request is currently in progress. module Filebeat Filebeat labels Dec 18, 2018
@webmat webmat changed the title WIP Replace all grok :int coercions with :long coercions Replace all grok :int coercions with :long coercions Dec 18, 2018
@webmat webmat added review and removed in progress Pull request is currently in progress. labels Dec 18, 2018
@webmat webmat requested a review from ruflin December 18, 2018 20:47
@webmat
Copy link
Contributor Author

webmat commented Dec 18, 2018

I've kicked the libbeat testsuite test a few times and it always fails with ERROR: for proxy_dep Container "..." is unhealthy. This is unrelated to the changes here

Copy link
Contributor

@andrewvc andrewvc left a comment

Choose a reason for hiding this comment

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

Seeing a long for an HTTP status makes me squirm, but reading through the discussion on the related issue I'm OK with it.

LGTM WFG. And it looks like all that's failing is the borked mac builds, so I guess we're just LGTM

@webmat
Copy link
Contributor Author

webmat commented Dec 19, 2018

@andrewvc Haha yeah I know it looks strange 😂

@webmat webmat merged commit 5d0d938 into elastic:master Dec 19, 2018
@webmat webmat deleted the fbmod-long-coercions branch December 19, 2018 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants