-
Notifications
You must be signed in to change notification settings - Fork 230
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
Fix handling unsupported compression codec (#795) #798
Conversation
Codecov Report
@@ Coverage Diff @@
## master #798 +/- ##
==========================================
- Coverage 97.89% 97.87% -0.03%
==========================================
Files 31 31
Lines 5279 5307 +28
==========================================
+ Hits 5168 5194 +26
- Misses 111 113 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
This pull request introduces 2 alerts when merging 6483a3d into fcee7d1 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging 0a5b0ba into fcee7d1 - view on LGTM.com new alerts:
|
@@ -73,6 +76,20 @@ class LegacyRecordBase: | |||
LOG_APPEND_TIME = 1 | |||
CREATE_TIME = 0 | |||
|
|||
def _assert_has_codec(self, compression_type): |
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.
Can we reuse the same function to avoid copy-paste?
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.
Sure. What module is the best place to put in?
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.
Oh, sorry. They will diverge after adding zstd support. I think that's the reason why it's duplicated in kafka-python: legacy record can't be used with zstd compression. But I didn't verified this, so the difference may be a mistake too.
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.
According to KIP-110
Zstd will only be allowed with magic = 2 format
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.
I see, let's see how it looks with zstd I guess.
Changes
Fixes #795
Checklist
CHANGES
folder