-
Notifications
You must be signed in to change notification settings - Fork 504
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
Add ZLIB dependency for compression in rocksdb #1078
Add ZLIB dependency for compression in rocksdb #1078
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PragmaTwice Good job! I tried to add Zlib support a few months ago but the macOS build didn't work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, sorry for didn't look the previous commits closely.
Never mind, this error will only appear when executed in a specific environment, not easy to find. |
Thanks all. Merging... |
@PragmaTwice Why we should support ZLIB, IIRC, ZLIB has poor performance, we already have there compression algorithms, snappy is general algorithm from leveldb period, not bad in everything, also is default compression in kvrocks. lz4 has powerful performance, and zstd has good compression ratio and not bad performance. So i think current compression algorithms are enough. |
I think there is no cost to support zlib since rocksdb already supports zlib. And if we really need high performance, we can just replace zlib with zlib-ng, which is several to ten times faster than zlib and compatible with APIs of zlib. |
@PragmaTwice That said, is there a real-world use case for this functionality? I ever saw a number of open-source projects failed by implementing whatever they can and ruined into a complexity monster. It may not be the case for every single commit but for their combination - If you aren't gonna need it, don't implement it. |
zlib is under the zlib license, and it is compatible to be included in an Apache 2.0 licensed project: https://www.apache.org/legal/resolved.html