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

Use api versions for new api calls (depends on #319) #320

Merged

Conversation

jbruggem
Copy link
Collaborator

@jbruggem jbruggem commented Oct 22, 2018

See #318.

This MR replaces server_0_p_10_p_1.ex by server_0_10_or_later.ex and uses api versions to handle the API calls implemented in it.

This would mean that from here on out, all messages of the API can be implemented without assuming the version of the server. All that is needed is to refer to the api_versions to check which is the greatest version of the message supported both by the server and the client.

Follow-up to #319

@jbruggem jbruggem force-pushed the use_api_versions_for_new_api_calls branch from 081de12 to 0f58fbe Compare November 6, 2018 13:33
@jbruggem jbruggem force-pushed the use_api_versions_for_new_api_calls branch from 0f58fbe to b33aa60 Compare November 6, 2018 14:46
@jbruggem
Copy link
Collaborator Author

jbruggem commented Nov 6, 2018

This MR is now ready for review !

(as usual, I think the CI tests need to be restarted to allow them to pass)

def api_versions_map(api_versions) do
api_versions
|> Enum.map(fn version -> {version.api_key, version} end)
|> Map.new
Copy link
Member

Choose a reason for hiding this comment

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

This function isn't supported by elixir 1.1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks, I'll fix that. Any idea what to replace it with ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nevermind :)

Copy link
Collaborator Author

@jbruggem jbruggem Nov 8, 2018

Choose a reason for hiding this comment

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

From the logs of the CI, it seems I fixed that problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another option

Enum.into(api_versions, %{}, fn version ->  {version.api_key, version} end)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks, I keep forgetting about Enum.into !

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.

This looks good to me.

@jbruggem
Copy link
Collaborator Author

jbruggem commented Nov 12, 2018

Hey people ! Anybody else apart from Joshua would like to give an opinion on this ? It is rather important in the sense that it alters a bit the way the project will implement future features, following the lead of Kafka since 0.10.

@joshuawscott joshuawscott merged commit a414095 into kafkaex:master Nov 12, 2018
@joshuawscott
Copy link
Member

@jbruggem Thanks again! I'll try to get a release together soon

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