-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(rpc): adding rpc call retrying #624
Conversation
Does this resolve #265? |
src/providers/mod.rs
Outdated
RpcError::UnsupportedProvider(provider.to_string()) | ||
}) | ||
for _ in 0..providers_to_iterate { | ||
let provider = keys.get(dist.sample(&mut OsRng)).ok_or_else(|| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noting that the same provider could be called multiple times. This risks a high priority provider going down and no alternative provider being attempted.
Here's a suggestion for an alternative that could be more robust:
- Pick the first provider with our current algo (weighted random)
- Excluding the first one, pick via weighted random. If only 1 provider available, pick the first one again
- Pick the first provider again
- Excluding the first and second one pick via weighted random. If only 2 are available, pick the second on again. If only 1 is available pick the first one again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the suggestion!
I was also thinking about how to improve this case when the same provider was sampled twice. I updated the PR with the removing from sampling provider that is already added to the list. So the fallback would be to a different available provider.
I'm thinking, is it makes sense to re-try the same provider (the case when there's only one provider for the chain). I think there should be some delay between re-trying the same provider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should work. I left a suggestion on a more robust algorithm.
|
||
// Update the weight of the provider to 0 to remove it from the next | ||
// sampling, as updating weights returns an error if | ||
// all weights are zero |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible all weights are already 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment! It's possible that all weights are 0 and this should catch at line 254.
We are skipping the last provider update to 0 (next line to this comment) so there are no error all weights are 0
on the update even if there is only one provider.
This is tested on two scenarios:
- One provider for the chain ID.
- Few providers for the chain ID.
- All providers are already 0 for the chain ID.
4b74afe
to
552188d
Compare
Description
This PR implements a retrying mechanism for RPC calls. This should be useful for the user experience when one of the providers returns a
rate-limited
error and we can retry up to max retries (currently is3
) or fall with the appropriate error if all providers are rate-limited.Metrics for attempts per chain ID are also added.
Resolve #265 and #452
How Has This Been Tested?
Due Diligence