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

Add log_start_offset to message protocol parsing #2020

Merged
merged 7 commits into from
Mar 25, 2020

Conversation

gabriel-tincu
Copy link
Contributor

@gabriel-tincu gabriel-tincu commented Mar 15, 2020

As mentioned in the commit, these need to come all together, as the future code also implements logic that will unpack depending on version. The other PR would then be the ZSTD code itself


This change is Reviewable

Copy link
Collaborator

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

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

A few things...

kafka/producer/sender.py Outdated Show resolved Hide resolved
kafka/protocol/produce.py Outdated Show resolved Hide resolved
kafka/protocol/produce.py Show resolved Hide resolved
requirements-dev.txt Outdated Show resolved Hide resolved
@jeffwidman jeffwidman changed the title Gabi kafka client zstd produce update Add log_start_offset to message protocol parsing Mar 16, 2020
@jeffwidman
Copy link
Collaborator

I'd like @dpkp and @tvoinarovskyi to take a look as well, particularly at the error handling bit.

@tvoinarovskyi
Copy link
Collaborator

@gabriel-tincu Great job on this, really appreciate your contribution.
Pointed 1 minor point that needs addressing, as @jeffwidman had doubts about. Other parts lgtm!

@gabriel-tincu gabriel-tincu force-pushed the gabi-kafka-client-zstd-produce-update branch from 32840cb to aaf61ff Compare March 19, 2020 20:53
@tvoinarovskyi
Copy link
Collaborator

Nice! LGTM

@tvoinarovskyi
Copy link
Collaborator

@jeffwidman Code looks good, but the current CI does not actually run 2.1 tests as API version checks are not up to date with the latest brokers. I will do a fix in separate PR, don't think it's a blocker.

Copy link
Collaborator

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

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

Looks great other than can you please add one inline comment for the _... once that's done I'll happily merge.

kafka/producer/sender.py Show resolved Hide resolved
@jeffwidman jeffwidman merged commit f9e0264 into dpkp:master Mar 25, 2020
@jeffwidman
Copy link
Collaborator

Thanks!

gabriel-tincu pushed a commit to aiven/kafka-python that referenced this pull request Sep 22, 2020
This is in preparation for adding `zstd` support.
@hackaugusto hackaugusto deleted the gabi-kafka-client-zstd-produce-update branch January 11, 2021 10:26
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