add exponential retry mechanism for rpc requests using utility function #593
Conversation
najeal
requested review from
iansuvak,
richardpringle,
geoff-vball,
bernard-avalabs,
michaelkaplan13 and
cam-schultz
There was a problem hiding this comment.
I left a handful of comments, but overall the backoff utility looks good.
There are a couple of other places that exponential backoff would make sense. Can you please integrate the utility in these places as well?
- p2p signature aggregation If this proves to be non-trivial, we can defer to a later ticket
- The existing call with retry utility and its callsites
| // WithMaxRetriesLog runs the operation until it succeeds or max retries has been reached. | ||
| // It uses exponential back off. | ||
| // It optionally logs information if logger is set. | ||
| func WithMaxRetriesLog( |
There was a problem hiding this comment.
Rather than separate with/without log functions, I think we should instead have a single method that takes a logging.Logger and emits warning logs on retries. I can't think of any use cases where we'd want retry attempts to be logged as Warn for some operations, but not logged at all for others.
| msg string, | ||
| fields ...zapcore.Field, |
There was a problem hiding this comment.
I think logging msg and fields on each attempt is not worth the added complexity of having to pass them in as arguments. Rather, WithMaxRetries should emit a generic warning log on each attempt failure, and we can leave it to the caller to construct a cohesive error log in the failure case.
| fields ...zapcore.Field, | ||
| ) error { | ||
| attempt := uint(1) | ||
| expBackOff := backoff.WithMaxRetries(backoff.NewExponentialBackOff(), max) |
There was a problem hiding this comment.
Let's use backoff.WithMaxElapsedTime instead. At present, we choose the number of retries and the delay between each to resolve to a set elapsed time before we emit an error.
There was a problem hiding this comment.
If I understand well, you want to use backoff.WithMaxElapsedTime instead of backoff.WithMaxRetries.
Instead of giving a maxRetry to the utility function, I will give a maxElapsedTime derived from the existing values (numberOfRetries * delayBetweenEach)
| r.logger.Warn( | ||
| "Failed to get aggregate signature from node endpoint", | ||
| zap.Int("attempts", maxRelayerQueryAttempts), | ||
| err = utils.WithMaxRetriesLog( |
There was a problem hiding this comment.
Just a head's up, the Warp API integration is planned to be deprecated in the near future. It's still reasonable to integrate exponential backoff here for the time being.
Why this should be merged
Relates to #453
How this works
Functions needing retry are passed to the utility function managing the retry mechanism.
How this was tested
unit test has been added for
WithMaxRetries()embeddingWithMaxRetriesLog()How is this documented