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

Increment attempt number instead of decrement when joining a consumer group #403

Merged
merged 2 commits into from
Apr 26, 2020

Conversation

mjparrott
Copy link
Contributor

Two things this fixes:

  1. The output of Unable to join consumer group "consumer_group". Will sleep 1 second and try again (attempt number x) should be less confusing, as it would start to become a negative number.

  2. attempt_number is used to raise an error when @max_join_retries is reached. This would never be reached when attempt_number is decrementing.

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.

Makes sense to me. I kicked travis to fix the one test. I'll merge once it passes

Copy link
Collaborator

@jbruggem jbruggem left a comment

Choose a reason for hiding this comment

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

Makes sense.

While you're at it, could you also maybe change the boundary check to be a bit more defensive ?

From :

if attempt_number == @max_join_retries do

To:

if attempt_number >= @max_join_retries do

Copy link
Collaborator

@dantswain dantswain left a comment

Choose a reason for hiding this comment

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

Great catch!

@jbruggem do you have any remaining concerns here?

@jbruggem jbruggem merged commit 1113a82 into kafkaex:master Apr 26, 2020
@jbruggem
Copy link
Collaborator

@dantswain thanks for the reminder.

@joshuawscott joshuawscott mentioned this pull request Jul 14, 2020
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