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

Ignore lookup_coordinator result in commit_offsets_async #1712

Merged
merged 2 commits into from
Mar 13, 2019

Conversation

asafflesch
Copy link
Contributor

@asafflesch asafflesch commented Feb 11, 2019

Fixes an issue where if commit_offsets_async has to lookup the coordinator before actually sending the offset_commit_request, the request will fail with the following error:

Error processing callback
Traceback (most recent call last):
  File "/home/runner/pipeline.runfiles/kafka_python_git/kafka/future.py", line 79, in _call_backs
    f(value)
TypeError: _do_commit_offsets_async() takes from 2 to 3 positional arguments but 4 were given

This is because the current logic directly adds the invocation of _do_commit_offsets_async as a callback to the invocation of lookup_coordinator, but _do_commit_offsets_async has no use for that value, and has no parameter to receive it, resulting in the above error. This PR just passes a wrapper lambda as the actual callback to the invocation of lookup_coordinator.


This change is Reviewable

… the actual call in a partial application to ignore the result from lookup_coordinator
@jeffwidman
Copy link
Collaborator

This makes sense to me.

@dpkp / @tvoinarovskyi your thoughts? If we are going to take this, it'd be nice to get it in before the next release.

@Faqa can you rebase/squash to a single commit?

Copy link
Collaborator

@tvoinarovskyi tvoinarovskyi left a comment

Choose a reason for hiding this comment

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

Can't this just be something like:
future.add_callback(lambda r: self._do_commit_offsets_async(offsets, callback))
More readable... Or does it not work?

@tvoinarovskyi
Copy link
Collaborator

Kind of find the partial part a bit hard to read.

@dpkp
Copy link
Owner

dpkp commented Mar 13, 2019

Thanks for the catch + bugfix!

@dpkp dpkp merged commit 994d283 into dpkp:master Mar 13, 2019
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.

4 participants