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

Cherry-pick #8278 to 6.x: Allow TCP helper to support delimiters #8302

Merged
merged 2 commits into from
Sep 13, 2018

Conversation

jsoriano
Copy link
Member

Cherry-pick of PR #8278 to 6.x branch. Original message:

There are TCP payloads coming in from graphite carbon forwarders into metricbeat that have many metrics separated by '\n`. What happens is that only the first event is parsed and the rest are dropped.

This PR attempts to allow TCP helper to accept a delimiter character which can be used to split the bytes coming in to the socket and create events for each one of them.


length, err := conn.Read(buffer)
go g.handle(conn)
Copy link
Contributor

Choose a reason for hiding this comment

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

In the original PR, this statement is wrapped in an if conn != nil. Is that needed here too?

func (g *TcpServer) handle(conn net.Conn) {
if conn == nil {
return
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This if block is not in the original PR. Any specific reason for the difference in the backport?

@jsoriano
Copy link
Member Author

@ycombinator thanks for reviewing this! And sorry for the confusion, I did a follow up PR (#8295) to the original one with some extra details, this one backports both PRs, this is why you see these differences.

In the follow up PR I modified the changelog entry to be more explicit on the use case it supports. I also removed the deferred function enclosing conn.Close() (as it can be called directly), and moved the check for the nil connection. I consider that in the caller we shouldn't expect it to be nil as we have just checked that Accept() returned no error. In handle() it can still make sense defensively, in case it is called from somewhere else with a nil connection.

Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

LGTM.

@jsoriano jsoriano merged commit 865a90a into elastic:6.x Sep 13, 2018
@jsoriano jsoriano deleted the backport_8278_6.x branch September 13, 2018 14:52
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