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

Do not update event in parallel #1428

Merged
merged 1 commit into from Apr 20, 2016

Conversation

Projects
None yet
2 participants
@urso
Copy link
Collaborator

commented Apr 19, 2016

logstash output used to add metadata to an event. With potentially having
multiple outputs configured this is some bad practice. Instead the logstash
output plugin will add '@metadata' to the event when encoding and only if
'@metadata' is not already present in event

Resolves #1410

buf.Write(tmp)

buf.WriteString(`,"beat":`)
buf.Write(beat)

This comment has been minimized.

Copy link
@tsg

tsg Apr 20, 2016

Collaborator

Shouldn't this also be passed through json.Marshal or otherwise escaped? We have to be careful with JSON injection if we're doing this by hand.

This comment has been minimized.

Copy link
@urso

urso Apr 20, 2016

Author Collaborator

json.Marshal is done line 62. Didn't want to marshal over and over again

@@ -285,6 +295,49 @@ func (p *protocol) serializeDataFrame(
return nil
}

func serializeEvent(event common.MapStr, beat []byte) ([]byte, error) {

This comment has been minimized.

Copy link
@tsg

tsg Apr 20, 2016

Collaborator

Some unit tests for this function would be good.

This comment has been minimized.

Copy link
@urso

urso Apr 20, 2016

Author Collaborator

it's tested (block-box testing sendEvents), as we've got some logstash unit tests forwarding events via sendEvents with server part decoding json messages for validation.

@tsg

This comment has been minimized.

Copy link
Collaborator

commented Apr 20, 2016

This looks to me like a good solution for the bug we have now, I guess long term we'll have a proper Event type and we can add locking or immutability to it, right?

@tsg

This comment has been minimized.

Copy link
Collaborator

commented Apr 20, 2016

LGTM. Can you add a changelog item?

@urso

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 20, 2016

I guess long term we'll have a proper Event type and we can add locking or immutability to it, right?

That's exactly what I have had in mind when doing the fix.

Do not update event in parallel
logstash output used to add metadata to an event. With potentially having
multiple outputs configured this is some bad practice. Instead the logstash
output plugin will add '@metadata' to the event when encoding and only if
'@metadata' is not already present in event

@urso urso force-pushed the urso:fix/1410-race-json-nil branch from b266a37 to 35163ae Apr 20, 2016

@tsg tsg merged commit 4e2223e into elastic:master Apr 20, 2016

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
CLA Commit author is a member of Elasticsearch
Details

@tsg tsg removed the needs_backport label May 9, 2016

@urso urso deleted the urso:fix/1410-race-json-nil branch May 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.