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 Datadog/zstd with Klauspost/compress #1383

Closed
wants to merge 17 commits into from

Conversation

jarifibrahim
Copy link
Contributor

@jarifibrahim jarifibrahim commented Jun 23, 2020

Edit - This is a stale PR. Please see #1706 for the latest one.

Fixes #1162
PR #1176 was the first attempt at replacing the library but we decided to not switch the library because of performance issues.

https://github.com/klauspost/compress has become much faster since the last time we benchmarked it in #1176. The new benchmarks in #1162 (comment) look very promising and we should get rid of the CGO dependency.


This change is Reviewable

@klauspost
Copy link

The block size of 4KB was converted to a size of 5554 bytes.

Let me double check. That shouldn't be possible.

y/zstd.go Outdated Show resolved Hide resolved
y/zstd.go Outdated Show resolved Hide resolved
Ibrahim Jarif and others added 2 commits June 23, 2020 20:05
Co-authored-by: Klaus Post <klauspost@gmail.com>
Co-authored-by: Klaus Post <klauspost@gmail.com>
@jarifibrahim jarifibrahim marked this pull request as ready for review June 29, 2020 13:38
@stale stale bot added the status/stale The issue hasn't had activity for a while and it's marked for closing. label Jul 29, 2020
@dgraph-io dgraph-io deleted a comment from stale bot Jul 29, 2020
@stale stale bot removed the status/stale The issue hasn't had activity for a while and it's marked for closing. label Jul 29, 2020
@jarifibrahim jarifibrahim added the skip/stale Skip stalebot label Jul 29, 2020
@ivanjaros
Copy link

ivanjaros commented Sep 13, 2020

Any progress with this?

If I run CGO_ENABLED=0 go get github.com/dgraph-io/badger/v2 from the readme I will get "cannot find package "github.com/dgraph-io/badger/v2" in any of... GOROOT, GOPATH and with cgo/standard I will get exec: "gcc": executable file not found in %PATH% since I do not have gcc. This used to work without any errors....so now I have to switch to BoltDB.

...
i noticed there is build tag in the y/* files so CGO_ENABLED=0 go build works fine. Maybe the readme should be updated.

@klauspost
Copy link

@jarifibrahim Ping. You might want to upgrade to v1.11.3, there are some minor tweaks and fixes. I don't see them invalidating the tests, but might as well get them in.

@emolitor
Copy link

emolitor commented Feb 6, 2021

Has this PR been abandoned? Slightly confused by the state of the ticket as reading through the comments it appears all the issues which were raised have been addressed. Are there other issues which are not documented or is this ticket just waiting for time by the author to test and review?

@mholt
Copy link

mholt commented May 10, 2021

We'd hate to have to rip this out of Caddy if this PR can't be merged.

@NamanJain8
Copy link
Contributor

@jarifibrahim closing this because the issue is addressed by #1706

@NamanJain8 NamanJain8 closed this May 28, 2021
@NamanJain8 NamanJain8 deleted the ibrahim/klauspost-compress branch May 28, 2021 06:37
@jarifibrahim
Copy link
Contributor Author

jarifibrahim commented May 28, 2021

We have merged #1706 which adds the pure go-based ZSTD to badger. This will be released as part of the badger release in July.

@mholt @emolitor @ivanjaros @klauspost Thank you all for your feedback and helping improve badger ❤️ . Apologies for the delay in merging this PR. It got lost in all the other work we're doing on badger/dgraph.

@francislavoie
Copy link

I don't think that PR actually solves the problem we have with badger re Caddy. The dependency is still part of the chain, so our users will still end up needing to explicitly specify CGO_ENABLED=0 (I think). We were hoping for this to not be necessary to specify.

@jarifibrahim
Copy link
Contributor Author

@francislavoie Okay. I think we can set pure go as the default one. I think @klauspost already mentioned that the klauspost/compress should be able to decompress data written by datadog/zstd. So we shouldn't have a problem.

@klauspost
Copy link

Yes and vice versa if that should become relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip/stale Skip stalebot
Development

Successfully merging this pull request may close these issues.

Use pure Go zstd implementation
7 participants