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

Remove cgo zstd package #1477

Merged
merged 4 commits into from
Sep 7, 2019
Merged

Remove cgo zstd package #1477

merged 4 commits into from
Sep 7, 2019

Conversation

klauspost
Copy link
Contributor

Switch from cgo package to pure Go implementation. This will (when fixed) work even if no cgo is present.

https://github.com/klauspost/compress/tree/master/zstd#zstd

Compression level is removed. It could be made so the first request determines the compression level. But since there is currently only two levels (fast and default) I am not sure it is worth it.

This does NOT fix #1252 - only updates the code used for compression/decompression.

Switch from cgo package to pure Go implementation. This will (when fixed) work even if no cgo is present.

https://github.com/klauspost/compress/tree/master/zstd#zstd

Compression level is removed. It could be made so the first request determines the compression level. But since there is currently only two levels (fast and default) I am not sure it is worth it.

This does NOT fix IBM#1252 - only updates the code used for compression/decompression.
@ghost ghost added the cla-needed label Sep 3, 2019
@klauspost
Copy link
Contributor Author

CLA signed...

@bai
Copy link
Contributor

bai commented Sep 4, 2019

Good timing, I just started to experiment with your lib yesterday to do exactly what this PR does. Thank you so much 👍

@d1egoaz
Copy link
Contributor

d1egoaz commented Sep 4, 2019

@d1egoaz
Copy link
Contributor

d1egoaz commented Sep 4, 2019

@bai might want to include your change to the build files, to disable explicitly cgo

@danielnelson
Copy link

We are running into issues with the new dependencies in influxdata/telegraf#6344 and this would solve that issue.

I notice there is a relatively large init function though: https://github.com/klauspost/compress/blob/master/zstd/fse_predefined.go#L72

If this could be ran on first use or manually that would be ideal for us, since a very low number of Telegraf users would use this functionality but everyone would need to run the init code on startup. I recognize this is mostly an issue with Telegraf but if these compression methods could be optional that would be very helpful for us.

@varun06
Copy link
Contributor

varun06 commented Sep 4, 2019

--- FAIL: TestMessageEncoding (0.04s)
12041    request_test.go:34: Encoding empty zstd failed
12042        got  [53 28 155 77 1 4 0 0 1 88 141 205 89 56 255 255 255 255 255 255 255 255] 
12043        want [180 172 84 179 1 4 0 0 1 88 141 205 89 56 255 255 255 255 0 0 0 9 40 181 47 253 32 0 1 0 0]

looks like a valid failure.

@klauspost
Copy link
Contributor Author

klauspost commented Sep 4, 2019

@varun06 Yeah, I guess it wants zero byte encoding to be have the complete wrapping. This just returns a 0 byte slice.

I will add an option to the encoder. Added PR: klauspost/compress#155

That said, expecting specific encoding in compression is perhaps a bit iffy. I will update it to the new values.

@danielnelson Actually not an unreasonable thing 👍 I will take a look. PR: klauspost/compress#156

@bai
Copy link
Contributor

bai commented Sep 6, 2019

Looks like the only thing that prevents this PR from being 🍏 is this newline that makes go fmt check fail: 68a97de. Proof on Travis: https://travis-ci.org/Shopify/sarama/builds/581646021.

Do you think you could remote that extra newline or cherry-pick my commit so we could get this merged? 🙏

@klauspost
Copy link
Contributor Author

@bai Sorry. I had forgot to set up go fmt. Fixed now.

@bai
Copy link
Contributor

bai commented Sep 7, 2019

Fantastic, thank you!

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.

Sending messages with ZStd compression enabled fails in multiple ways
6 participants