Skip to content
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

Retry on rpc timeout response #518

Merged

Conversation

gianfra-t
Copy link
Contributor

@gianfra-t gianfra-t commented Apr 30, 2024

Closes #516.

General Description

Upon receiving a RequestTimeout error, the client should retry sending the transaction. This will be done with the same backoff used for other errors defined as "Skippable".

Implementation

We add a method to identify the RequestTimeout error, and add it to the respective closures with the corresponding Skip retry policy.

The client's existing timeout (for some transactions) should also be larger (currently 5 minutes) than the one returned by the unresponsive chain. For requests we will just retry until the backoff is exausted.

@gianfra-t gianfra-t linked an issue Apr 30, 2024 that may be closed by this pull request
@gianfra-t
Copy link
Contributor Author

@pendulum-chain/devs Although I think this could solve the issue, I was not able to trigger a RequestTimeout on a modified local chain to test it.

@gianfra-t gianfra-t requested a review from a team April 30, 2024 19:18
@gianfra-t gianfra-t marked this pull request as ready for review April 30, 2024 19:18
Copy link
Contributor

@b-yap b-yap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No complaints from me.

Copy link
Member

@ebma ebma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me as well, good job @gianfra-t 👍

@gianfra-t
Copy link
Contributor Author

The CI keeps failing on me 😕. Someone knows why could this be?

@ebma
Copy link
Member

ebma commented May 3, 2024

I don't know about this error but the 'test_get_proof_for_current_slot failed' in this run is probably just due to a shaky test. The test_get_proof_for_current_slot() test is known to fail often in the CI 😞 I just restarted it again, let's see.

@gianfra-t
Copy link
Contributor Author

Yes one of the previous was a failed build, very strange since it has been built many times already!

@gianfra-t gianfra-t merged commit ea66a30 into main May 6, 2024
1 check passed
@gianfra-t gianfra-t deleted the 516-make-vault-client-retry-submissions-for-rpc-timeout-errors branch May 6, 2024 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make vault client retry submissions for RPC timeout errors
3 participants