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

[PR366] Make sure reconnectToBroker invoked before tearing down connection #373

Closed
wants to merge 2 commits into from

Conversation

zzzming
Copy link
Contributor

@zzzming zzzming commented Sep 28, 2020

<--

Contribution Checklist

Master Issue: #366

Motivation

We have seen consumer hanging issues reported similar to #366
Consumer was unable to reconnect to broker under these conditions

  • multiple consumers with shared subscriptions
  • topic is unloaded and reloaded to another broker due to load balancing

As @jiazhai pointed that partition consumer's reconnectToBroker() may have never been invoked due to deadlock of closed Connection. This is described at the end of PR 366 comments.

Modifications

Reorganize the code to ensure consumer and producer handlers are closed before the connection is torn down, so that broker reconnect mechanism can run independently to retry until successful or reach the retry limit.

Verifying this change

  • Make sure that the change passes the CI checks.
    This change is already covered by existing tests, such as (please describe tests).

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not applicable)
  • If a feature is not applicable for documentation, explain why?
    This is not a feature but a bug fix.

@wolfstudy
Copy link
Member

The issue will be fixed in #376, will close this pull request, thanks @zzzming work for this.

@wolfstudy wolfstudy closed this Oct 10, 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.

2 participants