[Issue 257][feat] Support limit the retry number of reconnectToBroker

[jonyhy96]

Fixes #257

Motivation

Once the connection is closed the reconnectToBroker logic of both producer and consumer will try to reconnect to the broker infinatly.

Modifications

Add a field in the Options represent the max number of retries and not break current behavior if this field is not fullfilled.

Verifying this change

• Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

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: (no)
• The schema: (yes)
• The default values of configurations: (no)
• The wire protocol: (no)

Documentation

• Does this pull request introduce a new feature? (yes)
• 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

[jonyhy96]

/cc @wolfstudy

[wolfstudy]

Thanks @jonyhy96 Here, I have only one confusion. When users use it, they need to set it on the producer and consumer at the same time, right? Can we consider exposing this parameter to the user in ClientOptions so that the user only needs to set it once

[jonyhy96]

Thanks @jonyhy96 Here, I have only one confusion. When users use it, they need to set it on the producer and consumer at the same time, right? Can we consider exposing this parameter to the user in ClientOptions so that the user only needs to set it once

That would be more convenience for users to use, But the user loses more detailed control over the program.
By the way,do you mean add an option at here and here,in reconnectToBroker use p.clinet.maxReconnectToBroker to decide the retry logic?

[jonyhy96]

Thanks @jonyhy96 Here, I have only one confusion. When users use it, they need to set it on the producer and consumer at the same time, right? Can we consider exposing this parameter to the user in ClientOptions so that the user only needs to set it once

That would be more convenience for users to use, But the user loses more detailed control over the program.
By the way,do you mean add an option at here and here,in reconnectToBroker use p.clinet.maxReconnectToBroker to decide the retry logic?

/cc @wolfstudy @shohi

Any further information about this? Maybe we can implement this feature before october 😄

[wolfstudy]

LGTM +1, just a little comment, @jonyhy96 please help check, thanks.

[jonyhy96]

LGTM +1, just a little comment, @jonyhy96 please help check, thanks.

Thanks for review! Already fix this.

[wolfstudy]

@jonyhy96 Can we not modify the original default behavior, and we will always retry by default. When we set the maximum number of retries, we will turn off the retry logic after reaching the specified threshold.

Because the current behavior will break the backward compatibility of the version

[jonyhy96]

Sure, neither do I want to break the current behavior. The original commit of this pr will fulfill our request, please check out the comments above. @shohi may have different opinion on this due to his(her) proposal. Please take a review.

[wolfstudy]

We can consider setting -1 as the default value, and then check whether MaxReconnectToBroker is set, and if so, use the user-defined value.

[wolfstudy]

At present, I see that you have added the option of MaxReconnectToBroker to the relevant test cases. This may not be very user-friendly. We need to ensure that the interface is as simple as possible.

[jonyhy96]

We can consider setting -1 as the default value, and then check whether MaxReconnectToBroker is set, and if so, use the user-defined value.

I understand,but how can we determine whether MaxReconnectToBroker is set or not when it's default value is meaningful. e.g. user set this option to 0.

At present, I see that you have added the option of MaxReconnectToBroker to the relevant test cases. This may not be very user-friendly. We need to ensure that the interface is as simple as possible.

I totally agree, how about we use the origin commit of this pr to implement this feature?

[wolfstudy]

ok, agree with you

[jonyhy96]

Should i reset this pr to original commit or keep it this way?

[wolfstudy]

@jonyhy96 Sorry for the reply later, i think we should reset this pull request to the original commit for backward compatibility.

[jonyhy96]

@jonyhy96 Sorry for the reply later, i think we should reset this pull request to the original commit for backward compatibility.

Already reset this pr to original commit.
By the way, the go.mod file seems a little bit dirty,you can reproduce this by run comamnd go mod tidy locally.

➜  pulsar-client-go git:(feat-max-reconnect-to-broker) go mod tidy -v        
unused github.com/modern-go/concurrent
unused github.com/modern-go/reflect2

perhaps add script below to ci for check if go.mod has been commit correctly

go mod tidy
git diff --exit-code -- go.mod go.sum