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

[Issue 496] Support for correct reconnections limit in consumer #497

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

omnilight
Copy link
Contributor

@omnilight omnilight commented Mar 28, 2021

Fixes #496

Motivation

Ability to set maximum number of reconnections to broker for consumer without consumer been stuck

Modifications

  • Added new method Closed() <-chan struct to consumer interface to be able to control if consumer is closed when using its Chan() method
  • For partitioned consumer added call of the parent's Close() method in case when number of reconnections is limited. This fixes problem when consumer stuck in disconnected state.

Verifying this change

This change added tests and can be verified as follows:

  • Added test for consumer closing on reaching max reconnection count
  • Added test for new Closed() method

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 documented)

Copy link
Member

@wolfstudy wolfstudy left a comment

Choose a reason for hiding this comment

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

@omnilight Thanks for work on this, when reconnectToBroker fails, we should close the parentConsumer object. This pr is for the consumer side. Will the producer have the same problem?

pulsar/consumer.go Show resolved Hide resolved
@wolfstudy wolfstudy modified the milestones: 0.6.0, 0.7.0 Jul 21, 2021
@omnilight omnilight force-pushed the fix_reconnection_limit_on_consumer branch from 5c57eae to 24c8863 Compare August 6, 2021 07:51
@omnilight
Copy link
Contributor Author

Added fix to prevent possible panic when pulsar returns error in consumer initialization step

@wolfstudy wolfstudy modified the milestones: 0.7.0, v0.8.0 Nov 1, 2021
@omnilight
Copy link
Contributor Author

@wolfstudy Hi, could you please review this PR?

@clawfinger
Copy link

Hello, do we have any ETA on this issue? Is it still planned for 0.8.0? Started using this lib recently and need this functionality badly

@rueian
Copy link
Contributor

rueian commented Jan 3, 2022

I also think this PR is important. I frequently find my consumer code stucked without knowing the underlying maximum number of reconnection had been reached.

@wolfstudy wolfstudy modified the milestones: v0.8.0, 0.9.0 Feb 16, 2022
@omnilight
Copy link
Contributor Author

@wolfstudy Could you please release this fix within the 0.9.0 release?

@omnilight
Copy link
Contributor Author

@wolfstudy Sorry for annoying you, but is it possible to include this PR into next release?

@omnilight
Copy link
Contributor Author

@wolfstudy Hi! Any changes should be made to accept this pr?

@freeznet freeznet modified the milestones: v0.9.0, v0.10.0 Jul 4, 2022
@RobertIndie RobertIndie modified the milestones: v0.10.0, v0.11.0 Mar 27, 2023
@RobertIndie RobertIndie modified the milestones: v0.11.0, v0.12.0 Jul 4, 2023
@RobertIndie RobertIndie modified the milestones: v0.12.0, v0.13.0 Jan 10, 2024
@RobertIndie RobertIndie modified the milestones: v0.13.0, v0.14.0 Jul 15, 2024
@omnilight
Copy link
Contributor Author

Hello!

Any chance this PR will be accepted?

@RobertIndie sorry for dm

@RobertIndie
Copy link
Member

RobertIndie commented Aug 20, 2024

Hello!

Any chance this PR will be accepted?

Hi @omnilight This PR looks good to me. But could you help resolve the conflicts? After that, I will review it again. Thanks.

@omnilight
Copy link
Contributor Author

But could you help resolve the conflicts? After that, I will review it again. Thanks.

Yep, will fix that soon

@omnilight
Copy link
Contributor Author

@RobertIndie Hi! I've rebases on latest master, please take a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MaxReconnectToBroker is useless for consumer
6 participants