chore(*): use proper retry #29
Conversation
There was a problem hiding this comment.
One note, babylon retry library have hardcoded errors which should/should not be retries:
var unrecoverableErrors = []error{
btclctypes.ErrHeaderParentDoesNotExist,
btclctypes.ErrChainWithNotEnoughWork,
btclctypes.ErrInvalidHeader,
btcctypes.ErrProvidedHeaderDoesNotHaveAncestor,
btcctypes.ErrInvalidHeader,
btcctypes.ErrNoCheckpointsForPreviousEpoch,
btcctypes.ErrInvalidCheckpointProof,
checkpointingtypes.ErrBlsPrivKeyDoesNotExist,
// TODO Add more errors here
}
// expectedErrors is a list of errors which can safely be ignored and should not be retried.
var expectedErrors = []error{
btcctypes.ErrDuplicatedSubmission,
btcctypes.ErrInvalidHeader,
// TODO Add more errors here
}
could you double check that this errors are relevant in this context ?
| }); err != nil { | ||
| }, | ||
| retry.Delay(m.ComCfg.RetrySleepTime), | ||
| retry.MaxDelay(m.ComCfg.MaxRetrySleepTime), |
There was a problem hiding this comment.
I would say we need to pass attempts config to every method, it would pretty wierd that some methods do not respect config value.
| opts = append(opts, opt) | ||
|
|
||
| err := retry.Do(retryableFunc, opts...) | ||
| if containsErr(expectedErrors, err) { |
There was a problem hiding this comment.
Why do we check the expected errors twice?
There was a problem hiding this comment.
@RafilxTenfen when retryif returns false, that means that Do is returning an error, we want to check if this error is expected and ignore it (return nil), as we don't want to keep retrying on expected errors or unrecoverable but in that case we do return an error. This is matching behavior of retry implementation in Babylon repo
There was a problem hiding this comment.
got it, thanks for explaining
There was a problem hiding this comment.
looks good, few minor comments.
One note for the future, for now it is fine to have this global list of errors, to not change behaviour, but ultimately this errors (unrecorevable/expected) should be set at call site by each caller. Then we know what call can return, what error and each caller knows what to do with each error. Maybe you could create task to get rid of those global error lists ?
| blockHash, err = c.GetBestBlockHash() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| }); err != nil { | ||
| }, | ||
| retry.Delay(c.retrySleepTime), |
There was a problem hiding this comment.
are those those options ever set in btclient ?
There was a problem hiding this comment.
Good catch, fixed in f7c96e3
| btccParamsRes, err = babylonClient.BTCCheckpointParams() | ||
| return err | ||
| }) | ||
| }, | ||
| retry.Delay(retrySleepTime), |
There was a problem hiding this comment.
I would say that, given max-attempts is in common potion of the config it should probably set in every program, otherwise we risk seeing some wierd behaviour only to learn some toold respect this config option and some does not
There was a problem hiding this comment.
Agreed added it in f7c96e3
Replaces usage of
retry.dofrom babylon withavast/retry-goReferences issue