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

Upgrade to lz4 1.10 and fix 4096 byte compression #19

Merged
merged 1 commit into from
Sep 21, 2024

Conversation

matthewberends
Copy link
Contributor

Changes

  • Update to lz4 to 1.10
  • Fix bug where compression would fail for batch sizes just under 4096 bytes.

Bug Description

  • From the compress_frame function @dstCapacity MUST be >= LZ4F_compressFrameBound(srcSize, preferencesPtr).
    However for message close to 4096 bytes, it looks like it is possible for slight expansion. Running a quick test with the message batch just under 4096:
    • The raw data size is 4086 (so under the 4096) threshold so allocates 4096 buffer size.
    • Running the same batch through LZ4F_compressFrameBound returns a result of 4113
    • This means @dstCapacity MUST be >= LZ4F_compressFrameBound(srcSize, preferencesPtr) is not met and 💣 💥 with a ERROR_dstMaxSize_tooSmall error

@qzhuyan qzhuyan merged commit e74500e into kafka4beam:master Sep 21, 2024
5 checks passed
@matthewberends
Copy link
Contributor Author

@qzhuyan thanks for the review. Is there a release planned with these changes

@qzhuyan
Copy link
Collaborator

qzhuyan commented Sep 23, 2024

tagged 0.0.12, publishing to hex is coming soon.

@qzhuyan
Copy link
Collaborator

qzhuyan commented Sep 23, 2024

hex.pm is broke for me, not release on hex.pm this time.

@zmstone
Copy link
Contributor

zmstone commented Sep 28, 2024

@qzhuyan if you make me a publisher, i can try to publish it.

@qzhuyan
Copy link
Collaborator

qzhuyan commented Sep 30, 2024

I have no permission to add publisher in hex.pm.

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.

3 participants