Skip to content

Add log_start_offset to message protocol parsing#2020

Merged
jeffwidman merged 7 commits intodpkp:masterfrom
aiven:gabi-kafka-client-zstd-produce-update
Mar 25, 2020
Merged

Add log_start_offset to message protocol parsing#2020
jeffwidman merged 7 commits intodpkp:masterfrom
aiven:gabi-kafka-client-zstd-produce-update

Conversation

@gabriel-tincu
Copy link
Copy Markdown
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
Copy Markdown
Contributor

@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...

Comment thread kafka/producer/sender.py Outdated
Comment thread kafka/protocol/produce.py
Comment thread kafka/protocol/produce.py
Comment thread requirements-dev.txt Outdated
@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
Copy Markdown
Contributor

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

@tvoinarovskyi
Copy link
Copy Markdown
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!

Comment thread kafka/producer/sender.py Outdated
@gabriel-tincu gabriel-tincu force-pushed the gabi-kafka-client-zstd-produce-update branch from 32840cb to aaf61ff Compare March 19, 2020 20:53
Comment thread kafka/producer/sender.py Outdated
@tvoinarovskyi
Copy link
Copy Markdown
Collaborator

Nice! LGTM

@tvoinarovskyi
Copy link
Copy Markdown
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
Copy Markdown
Contributor

@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.

Comment thread kafka/producer/sender.py
@jeffwidman jeffwidman merged commit f9e0264 into dpkp:master Mar 25, 2020
@jeffwidman
Copy link
Copy Markdown
Contributor

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