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

fix: remove NewConsumerFromClient method call in newConsumerGroup method #2423

Closed
wants to merge 3 commits into from

Conversation

Lumotheninja
Copy link
Contributor

@Lumotheninja Lumotheninja commented Jan 19, 2023

fix the issue of consumergroup not closing properly when calling consumergroup.close()

currently, the NewConsumerGroup method calls the NewConsumerFromClient method which creates a nopclient, and thus this client cannot be closed properly

Screenshot 2023-01-19 at 5 28 15 PM

Screenshot 2023-01-19 at 5 24 18 PM

Screenshot 2023-01-19 at 5 24 35 PM

consumer_group.go Outdated Show resolved Hide resolved
@dnwe
Copy link
Collaborator

dnwe commented Jan 19, 2023

@Lumotheninja thanks for catching this problem, see query above.

Also, please could you also fix your author settings in gitconfig to have a name and e-mail address matching your GitHub username and then sign the CLA to have your PR reviewed.

@Lumotheninja
Copy link
Contributor Author

@Lumotheninja thanks for catching this problem, see query above.

Also, please could you also fix your author settings in gitconfig to have a name and e-mail address matching your GitHub username and then sign the CLA to have your PR reviewed.

Hi @dnwe sorry this is my first time contributing to Sarama, I have signed the CLA as well as added my email to the last commit, is that sufficient?

@Lumotheninja
Copy link
Contributor Author

Lumotheninja commented Jan 19, 2023

I think I am unable to change the email of past commiters, let me open a new PR

please see #2424

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