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 private map of api key -> min/max versions to BrokerConnection #1169

Merged
merged 1 commit into from
Aug 7, 2017

Conversation

dpkp
Copy link
Owner

@dpkp dpkp commented Aug 5, 2017

This is a step towards #865 . For now there is no change to behavior. We might consider faking the min/max data for old broker versions. I haven't looked at the java client to see how they handle this map, but this seems like a fairly easy step to at least get some logic into the connection object.

@@ -6,3 +6,40 @@
CODEC_NONE, CODEC_GZIP, CODEC_SNAPPY, ALL_CODECS,
ATTRIBUTE_CODEC_MASK, KafkaProtocol,
)

API_KEYS = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the use case for this mapping? I could only think of logging...

Copy link
Owner Author

Choose a reason for hiding this comment

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

TBH, I created it primarily just to document the values. But there is some potential to switch the values from strings to the versioned list of class structs we have and use it for decoding api messages. That's obviously not in this PR though.

@tvoinarovskyi
Copy link
Collaborator

LGTM
As for how Java handles it, a brief overview:

@dpkp dpkp merged commit da25df6 into master Aug 7, 2017
@dpkp dpkp deleted the handle_api_versions branch August 13, 2017 20:10
88manpreet pushed a commit to Yelp/kafka-python that referenced this pull request Aug 25, 2017
88manpreet pushed a commit to Yelp/kafka-python that referenced this pull request Oct 6, 2017
88manpreet pushed a commit to Yelp/kafka-python that referenced this pull request Jul 16, 2018
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.

2 participants