-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
changefeedccl: fix kafka v2 sink does not allow negative GZIP compression levels less than zero #137646
base: master
Are you sure you want to change the base?
Conversation
Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Thank you for updating your pull request. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Thank you for updating your pull request. Before a member of our team reviews your PR, I have some potential action items for you:
🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Thanks for the contribution @yaothao. Can you take a look at what the bot has said here: #137646 (comment) You may also want to check out our guidelines for commit messages: https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/73072807/Git+Commit+Messages |
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.
This looks good, just need to fix the things i mentioned in my last comment
Previously, the Kafka v2 sink could not properly handle negative compression levels due to differences in the underlying compression libraries used between the v1 and v2 sinks. The Kafka v1 sink implementation, which relies on the Sarama library, uses the klauspost/compress library that supports a compression level of -3. However, our v2 sink has transitioned to using franz-go, which utilizes the standard library's compression/gzip, and does not support the -3 level. In this update, the validation function now checks the GZIP compression range between HuffmanOnly (-2) and BestCompression (9). Fixes: cockroachdb#136492 Epic: none Release note (bug fix): We have resolved an issue in the Kafka v2 sink configuration within CockroachDB, where users were previously unable to set negative GZIP compression levels. Now, users can configure the CompressionLevel for the Kafka sink in the range of [-2, 9]. Please update the user guide to include the new valid GZIP compression level range of [-2, 9], where -2 enables Huffman encoding and -1 sets the default compression.
Thank you for your feedback, @asg0451. I have incorporated several suggestions from our Git Commit Messages guidelines. Regarding the comments on this pull request, I have already addressed the issue with multiple commits and made the necessary adjustments. As I continue to learn through the process, please let me know if there's anything else I might have missed or could enhance. Thank you for your time. |
Fix: Support for Negative GZIP Compression Levels in Kafka v2 Sink #136492
This update fixes a bug in the Kafka v2 sink where negative GZIP compression levels were not supported within the expected range of
[-2, 9]
. Previously, the v2 sink could not handle negative compression levels properly due to differences in the underlying compression libraries used between v1 and v2 sinks.Background
Our Kafka v1 sink implementation, which relies on the
sarama
library, uses theklauspost/compress
library that supports a compression level of-3
. However, our v2 sink has transitioned to usingfranz-go
, which utilizes the standard library'scompression/gzip
, and does not support the-3
level. This discrepancy led to confusion and a bug when using similar configurations across different sink versions.Changes Made
[-2, 9]
, aligning with the capabilities of the Go standard library’s gzip package.Documentation Changes Required
-3
from the supported levels in the Kafka v2 sink documentation.Testing