[pulsar-client] Add conf backoff values #12520
Conversation
| @@ -884,9 +884,9 @@ public void reloadLookUp() throws PulsarClientException { | |||
| TopicName topicName = TopicName.get(topic); | |||
| AtomicLong opTimeoutMs = new AtomicLong(conf.getLookupTimeoutMs()); | |||
| Backoff backoff = new BackoffBuilder() | |||
| .setInitialTime(100, TimeUnit.MILLISECONDS) | |||
| .setInitialTime(conf.getInitialBackoffIntervalNanos, TimeUnit.NANOSECONDS) | |||
There was a problem hiding this comment.
Nice catch, looks like we apply the configuration for producer and consumer but missed the client part.
Could you please help add a test for covering the changes to make sure we will not make a regression in the future?
There was a problem hiding this comment.
@codelipenghui - what would you like to see tested? I think it is pretty well defined to use the config for the backoff here. My only concern would be backwards compatibility, but I don't see that being a real issue. The default config here is 100 milliseconds.
There was a problem hiding this comment.
@michaeljmarshall I mean to add a test to cover the backoff interval is configurable.
There was a problem hiding this comment.
@codelipenghui - thanks for clarifying. I'm not exactly sure how to add that test, but I think we could probably merge this as is.
codelipenghui
added
release/2.8.3
release/2.9.1
type/bug
There was a problem hiding this comment.
@callumduffy - thanks for your contribution. I noticed that ConsumerImpl#internalGetLastMessageIdAsync() also has the value for initialTime hard coded. Would you like to update that configuration as well?
pulsar-client/src/main/java/org/apache/pulsar/client/impl/conf/ClientConfigurationData.java
Outdated
Show resolved
Hide resolved
pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java
Outdated
Show resolved
Hide resolved
pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java
Outdated
Show resolved
Hide resolved
| @@ -884,9 +884,9 @@ public void reloadLookUp() throws PulsarClientException { | |||
| TopicName topicName = TopicName.get(topic); | |||
| AtomicLong opTimeoutMs = new AtomicLong(conf.getLookupTimeoutMs()); | |||
| Backoff backoff = new BackoffBuilder() | |||
| .setInitialTime(100, TimeUnit.MILLISECONDS) | |||
| .setInitialTime(conf.getInitialBackoffIntervalNanos, TimeUnit.NANOSECONDS) | |||
There was a problem hiding this comment.
@codelipenghui - what would you like to see tested? I think it is pretty well defined to use the config for the backoff here. My only concern would be backwards compatibility, but I don't see that being a real issue. The default config here is 100 milliseconds.
* add conf backoff values * Apply suggestions from code review Co-authored-by: Callum Duffy <callum.duffy@toasttab.com> Co-authored-by: Michael Marshall <mikemarsh17@gmail.com> ### Motivation Allowing services waiting on pulsar container etc to use the backoff values given in the config when the client is initialised. Using a service where i handle the future on my side, but i feel this should be in the service as is! ### Modifications Edit the values used for the backoff to use the client values. Defaults are the exact same as the values used previous to this change. (cherry picked from commit 970363c)
* add conf backoff values * Apply suggestions from code review Co-authored-by: Callum Duffy <callum.duffy@toasttab.com> Co-authored-by: Michael Marshall <mikemarsh17@gmail.com> ### Motivation Allowing services waiting on pulsar container etc to use the backoff values given in the config when the client is initialised. Using a service where i handle the future on my side, but i feel this should be in the service as is! ### Modifications Edit the values used for the backoff to use the client values. Defaults are the exact same as the values used previous to this change. (cherry picked from commit 970363c)
* add conf backoff values * Apply suggestions from code review Co-authored-by: Callum Duffy <callum.duffy@toasttab.com> Co-authored-by: Michael Marshall <mikemarsh17@gmail.com> ### Motivation Allowing services waiting on pulsar container etc to use the backoff values given in the config when the client is initialised. Using a service where i handle the future on my side, but i feel this should be in the service as is! ### Modifications Edit the values used for the backoff to use the client values. Defaults are the exact same as the values used previous to this change.
Motivation
Allowing services waiting on pulsar container etc to use the backoff values given in the config when the client is initialised. Using a service where i handle the future on my side, but i feel this should be in the service as is!
Modifications
Edit the values used for the backoff to use the client values.
Defaults are the exact same as the values used previous to this change.
Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Does this pull request potentially affect one of the following parts:
If
yeswas chosen, please highlight the changesDocumentation
Need to update docs?
doc-required(If you need help on updating docs, create a doc issue)
no-need-doc(Please explain why)
doc(If this PR contains doc changes)