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

Implement ApiVersions message in protocol #319

Merged
merged 3 commits into from
Nov 6, 2018

Conversation

jbruggem
Copy link
Collaborator

Closes #318 .

The support for ApiVersions was actually added in 0.10.0.0, but here I'm adding it to 0.10.1.0 for the sake of simplicity because it's temporary.

I intend to follow this PR with another where I get rid of server_0_p_10_p_1.ex in favor of server_0_10_or_later.ex (or some other better name), and then use ApiVersions to check compatibility at runtime.

@jbruggem
Copy link
Collaborator Author

Tests pass (though an admin might need to restart some of them)

@jbruggem
Copy link
Collaborator Author

The follow-up is ready :). It depends on this one so it is based on the same branch.

As soon is this one is reviewed and merged, I can open the other one.

If anybody is interested: https://github.com/euranova/kafka_ex/tree/use_api_versions_for_new_api_calls

@joshuawscott
Copy link
Member

@jbruggem Sorry about the delay in looking at this, I'll probably not be able to take a look until Thursday due to life interfering 😢

lib/kafka_ex/protocol/api_versions.ex Outdated Show resolved Hide resolved
lib/kafka_ex.ex Outdated Show resolved Hide resolved
lib/kafka_ex/server_0_p_10_p_1.ex Show resolved Hide resolved
@joshuawscott
Copy link
Member

@jbruggem this is looking good, just a few small fixes and it should be ready to merge

@jbruggem
Copy link
Collaborator Author

jbruggem commented Nov 2, 2018

@jbruggem this is looking good, just a few small fixes and it should be ready to merge

great! Thanks for taking the time to review.

Copy link
Member

@joshuawscott joshuawscott left a comment

Choose a reason for hiding this comment

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

👍

@jbruggem
Copy link
Collaborator Author

jbruggem commented Nov 6, 2018

@joshuawscott thanks for the second review :). If you feel it's OK, would you or another maintainer be kind enough to merge ?

Thanks !

@joshuawscott joshuawscott merged commit ef63708 into kafkaex:master Nov 6, 2018
@joshuawscott
Copy link
Member

@jbruggem sorry about the delay, just making sure none of the other maintainers had any comments

@jbruggem
Copy link
Collaborator Author

jbruggem commented Nov 6, 2018

@jbruggem sorry about the delay, just making sure none of the other maintainers had any comments

No problem !

joshuawscott added a commit that referenced this pull request Nov 12, 2018
Use api versions for new api calls (depends on #319)
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