Allow configuring max retries #84
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rhinof
reviewed
Jun 5, 2019
gbus/abstractions.go
Outdated
| @@ -166,6 +166,9 @@ type Builder interface { | |||
| //ConfigureHealthCheck defines the default timeout in seconds for the db ping check | |||
| ConfigureHealthCheck(timeoutInSeconds time.Duration) Builder | |||
|
|
|||
| //RetriesNum defines the number of retries upon error | |||
| RetriesNum(retries uint) Builder | |||
There was a problem hiding this comment.
As there are going to be more configuration options that are currently hard coded but moving forward we would like to provide a way to configure I think we would probably be better off have one WithConfiguration(ConfigObject) that we can use to place on it additional config options moving forward.
WDYT ?
rhinof
reviewed
Jun 5, 2019
gbus/builder/builder.go
Outdated
| @@ -170,6 +170,11 @@ func (builder *defaultBuilder) ConfigureHealthCheck(timeoutInSeconds time.Durati | |||
| return builder | |||
| } | |||
|
|
|||
| func (builder *defaultBuilder) RetriesNum(retries uint) gbus.Builder { | |||
| gbus.MaxRetryCount = retries | |||
There was a problem hiding this comment.
should guard against zero and negative values
vladshub
reviewed
Jun 5, 2019
gbus/builder/builder.go
Outdated
| @@ -170,6 +170,11 @@ func (builder *defaultBuilder) ConfigureHealthCheck(timeoutInSeconds time.Durati | |||
| return builder | |||
| } | |||
|
|
|||
| func (builder *defaultBuilder) RetriesNum(retries uint) gbus.Builder { | |||
| gbus.MaxRetryCount = retries | |||
There was a problem hiding this comment.
Suggested change
| gbus.MaxRetryCount = retries | |
| if retries > 0 { | |
| gbus.MaxRetryCount = retries | |
| } |
rhinof
reviewed
Jun 5, 2019
gbus/worker.go
Outdated
| return retry.Retry(action, | ||
| strategy.Limit(MaxRetryCount), | ||
| strategy.Backoff(backoff.Fibonacci(50*time.Millisecond))) | ||
| strategy.BackoffWithJitter( | ||
| backoff.BinaryExponential(10*time.Millisecond), |
There was a problem hiding this comment.
lets have these values exposed via configuration with default values
There was a problem hiding this comment.
just the 10*time.Millisecond, right?
rhinof
suggested changes
Jun 6, 2019
gbus/abstractions.go
Outdated
| @@ -15,6 +15,12 @@ const ( | |||
| EVT Semantics = "evt" | |||
| ) | |||
|
|
|||
| //ConfigObject provides configuration passed to the bus builder | |||
| type ConfigObject struct { | |||
There was a problem hiding this comment.
Please rename to BusConfiguration
rhinof
approved these changes
Jun 6, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
retries
Provide a way to externally configure the maximum amount of retries handlers get executed before declaring the message as poison and rejecting it
Closes #35