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.

Sign up for GitHub

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

Jump to bottom

Correctly respect timeouts in consumer poll interface #1384

Merged
merged 1 commit into from
Feb 21, 2018
Merged

Correctly respect timeouts in consumer poll interface #1384

merged 1 commit into from
Feb 21, 2018

Conversation

braedon
Copy link
Contributor

@braedon braedon commented Feb 16, 2018 •
edited
Loading

ConsumerCoordinator.time_to_next_poll() returns seconds, so need to convert to milliseconds.

Fixes #1383

@braedon
Correctly respect timeouts in consumer poll interface
05920f2 
ConsumerCoordinator.time_to_next_poll() returns seconds, so need to convert
to milliseconds.

Fixes dpkp#1383
@tvoinarovskyi
Copy link
Collaborator

LGTM will try to get the build green and ready to go 🚢

@tvoinarovskyi tvoinarovskyi requested a review from dpkp February 19, 2018 13:34
jeffwidman
jeffwidman approved these changes Feb 21, 2018
View reviewed changes
Copy link
Collaborator

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

LGTM. Only question is if it's appropriate for time_to_next_poll() to return seconds or should it be inherently returning milliseconds? IE, should this be patched here or further upstream?

@tvoinarovskyi
Copy link
Collaborator

In kafka-python we have the pattern all over the place. Ie, it returns seconds, not milliseconds. I think it's ok.

@dpkp
Copy link
Owner

dpkp commented Feb 21, 2018

LGTM

@dpkp dpkp merged commit 92635d9 into dpkp:master Feb 21, 2018
@braedon braedon deleted the 1383_fix_consumer_poll_timeout branch March 26, 2019 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tvoinarovskyi tvoinarovskyi tvoinarovskyi approved these changes

@jeffwidman jeffwidman jeffwidman approved these changes

@dpkp dpkp Awaiting requested review from dpkp

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@braedon @tvoinarovskyi @dpkp @jeffwidman