-
Notifications
You must be signed in to change notification settings - Fork 4.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
kafka: 2.4.0 support - add support for new data types added in 2.4 #9813
Conversation
a1fe3c1
to
2f0d1a9
Compare
…egers, compact strings, compact bytes, compact arrays Signed-off-by: Adam Kotwasinski <[email protected]>
Signed-off-by: Adam Kotwasinski <[email protected]>
Signed-off-by: Adam Kotwasinski <[email protected]>
2f0d1a9
to
862482c
Compare
Signed-off-by: Adam Kotwasinski <[email protected]>
Signed-off-by: Adam Kotwasinski <[email protected]>
Signed-off-by: Adam Kotwasinski <[email protected]>
Signed-off-by: Adam Kotwasinski <[email protected]>
@adamkotwasinski are you waiting on a review here, or still iterating? |
@junr03 still iterating, didn't expect that build failure! |
…variable-length uint deserializer Signed-off-by: Adam Kotwasinski <[email protected]>
Signed-off-by: Adam Kotwasinski <[email protected]>
Signed-off-by: Adam Kotwasinski <[email protected]>
/wait-any |
This PR should not have any impact on existing Kafka protocol coverage, as the new classes are not yet referenced from (generated) requests.h / responses.h protocol files. It can be noticed that there's some duplication here, as we are handling very similar data types all the time (e.g. (nullable) (compact) byte array). I will want to revisit this place to refactor it later, to avoid PR noise. /assign @mattklein123 |
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 from a skim, thanks. (Note that as we get more people using this in production we are going to need to find more people to do low level code reviews.)
@adamkotwasinski will the dependency updates come in a follow up PR? |
@moderation yes - when I'm done with #10000 |
Description:
Provide codec support for data types added in 2.4.0.
In detail, we Kafka 2.4 added some new things in protocol:
This PR should not have any impact on existing codebase, just the volume of changes (if everything was posted at once) is IMHO too large to comfortably review.
Relates to #2852
These deserializers are working with 2.4 request types (they are not part of this PR as I'm still working on that and want to make another PR with them).
Risk Level: Low
Testing: Automated unit tests; embedded integration test (ensures compatibility with 2.3 client); manual tests with 2.3 client
Docs Changes: N/A
Release Notes: N/A