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] Reconnection logic and Backoff policy doesn't work correctly #1197

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

Conversation

crossoverJie
Copy link
Contributor

Fixes #1187

Modifications

  • Move backoff.go to the backoff directory (because there are circular dependencies, they are not moved to the pulsar directory.)
  • Create a new method for BackOffPolicy interface IsMaxBackoffReached(delayReconnectTime, totalDelayReconnectTime time.Duration) bool

This is a breaking change that modifies the package name and interface name.

Package: internal->backoff
Interface name: BackoffPolicy-> Policy

Verifying this change

  • Make sure that the change passes the CI checks.

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

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (yes)
  • 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? (GoDocs)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

@crossoverJie
Copy link
Contributor Author

@RobertIndie PTAL

@RobertIndie RobertIndie self-requested a review March 21, 2024 14:24
@RobertIndie RobertIndie added this to the v0.13.0 milestone Mar 21, 2024
Copy link
Member

@RobertIndie RobertIndie left a comment

Choose a reason for hiding this comment

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

I'm concerned that such breaking changes could significantly impact users. Users would have to implement the new methods introduced by the backoff policy, even if they haven't encountered this issue. Perhaps we could consider including this change in the 1.0 version.

I have initiated a discussion of the Go client 1.0.0: https://lists.apache.org/thread/h022q6nrk23tz2k0qpxjf6j8m1o4qsrt

@crossoverJie
Copy link
Contributor Author

Perhaps we could consider including this change in the 1.0 version.

Yes, I agree with you.

delayReconnectTime = pc.options.backoffPolicy.Next()
}
delayReconnectTime = bo.Next()
totalDelayReconnectTime += delayReconnectTime
Copy link

@chinmayb chinmayb May 29, 2024

Choose a reason for hiding this comment

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

We also need to reset the delayReconnectTime once connected after several retries in 1699.

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.

[BUG] Reconnection logic and Backoff policy doesn't work correctly
3 participants