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

dekaf: Swap to lz4_flex from lz4 #1653

Merged
merged 1 commit into from
Sep 23, 2024
Merged

dekaf: Swap to lz4_flex from lz4 #1653

merged 1 commit into from
Sep 23, 2024

Conversation

jshearer
Copy link
Contributor

@jshearer jshearer commented Sep 23, 2024

While investigating the cause of LZ4 compression issues related to franz-go (#1651 (comment)), I found lz4_flex which is a pure-Rust lz4 implementation which appears to be safer and faster than lz4/lz4-sys that kafka-protocol is using.

Now that tychedelia/kafka-protocol-rs#81 allows us to use our own compression, and lz4's configuration of block checksums is broken (fix here 10XGenomics/lz4-rs#52), I thought it would be a good time to swap to lz4_flex.

I've confirmed that this change also fixes the issue with block checksums, mainly because lz4_flex's block checksums are actually off by default (unfortunately, turning them on reproduces the issue).


This change is Reviewable

While investigating the cause of LZ4 compression issues related to franz-go (see comments here #1651), I found `lz4_flex` which is a pure-Rust lz4 implementation which appears to be safer and faster than `lz4`/`lz4-sys` that `kafka-protocol` is using. Now that tychedelia/kafka-protocol-rs#81 allows us to use our own compression, and `lz4`'s configuration of block checksums is broken (fix here 10XGenomics/lz4-rs#52), I thought it would be a good time to swap to `lz4_flex`.
Copy link
Member

@jgraettinger jgraettinger left a comment

Choose a reason for hiding this comment

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

LGTM

@jshearer jshearer merged commit 51c253a into master Sep 23, 2024
5 checks passed
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.

2 participants