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

fix: Correct records sent to Kinesis Data Streams #13

Merged
merged 4 commits into from
May 23, 2018

Conversation

mumoshu
Copy link
Collaborator

@mumoshu mumoshu commented May 22, 2018

glide up wasnt necessarily but anyway I verified this to work with the latest version of aws-sdk-go

Fixes #11

Depends on #12. Plese see the last commit to review the change made in this PR.

mumoshu added 3 commits May 22, 2018 19:16
…a Streams

This should be enhanced to cover firehose and other beats. This is just a starting point :)
Fixes s12v#11

`glide up` wasnt necessarily but anyway I verified this to work with the latest version of aws-sdk-go
@mumoshu mumoshu force-pushed the fix-stream-record-corruption branch from e48c1c3 to cd1aafe Compare May 22, 2018 12:44
@codecov-io
Copy link

codecov-io commented May 22, 2018

Codecov Report

Merging #13 into master will increase coverage by 3%.
The diff coverage is 53.84%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #13   +/-   ##
=======================================
+ Coverage   21.73%   24.74%   +3%     
=======================================
  Files           3        3           
  Lines          92       97    +5     
=======================================
+ Hits           20       24    +4     
- Misses         67       68    +1     
  Partials        5        5
Impacted Files Coverage Δ
streams/client.go 21.05% <53.84%> (+4.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 26fbf25...350b099. Read the comment docs.

@di1214
Copy link

di1214 commented May 22, 2018

@mumoshu can you add the fix to firehose as well?

@mumoshu
Copy link
Collaborator Author

mumoshu commented May 23, 2018

@di1214 Sure! But it is really up to @s12v whether the fix is actually merged or not. He is the author of awsbeats. I don't have write permission to this repository 😉

// See https://github.com/elastic/beats/blob/5a6630a8bc9b9caf312978f57d1d9193bdab1ac7/libbeat/outputs/kafka/client.go#L163-L164
// You need to copy the byte data like this. Otherwise you see strange issues like all the records sent in a same batch has the same Data.
buf = make([]byte, len(serializedEvent))
copy(buf, serializedEvent)
Copy link
Owner

Choose a reason for hiding this comment

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

👍

@s12v s12v merged commit 0616dae into s12v:master May 23, 2018
@mumoshu mumoshu deleted the fix-stream-record-corruption branch May 23, 2018 07:44
mumoshu added a commit to mumoshu/awsbeats that referenced this pull request May 23, 2018
This is the Dockerfile referenced in README, which is verified to work to build filebeat.
Probably this can be enhanced to also support building other beats.

Follow up for s12v#12 and s12v#13. Missed to include this in those PRs :)
mumoshu added a commit to mumoshu/awsbeats that referenced this pull request May 23, 2018
This is the Dockerfile referenced in README, which is verified to work to build filebeat.
Probably this can be enhanced to also support building other beats.

Follow up for s12v#12 and s12v#13. Missed to include this in those PRs :)
@mumoshu mumoshu mentioned this pull request May 23, 2018
mumoshu added a commit that referenced this pull request May 23, 2018
This is the Dockerfile referenced in README, which is verified to work to build filebeat.
Probably this can be enhanced to also support building other beats.

Follow up for #12 and #13. Missed to include this in those PRs :)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants