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 calls to execute as well as to query #577

Merged
merged 2 commits into from
Feb 13, 2025
Merged

Retry calls to execute as well as to query #577

merged 2 commits into from
Feb 13, 2025

Conversation

msullivan
Copy link
Member

In the distant past, execute was quite different than query (execute
couldn't take arguments and query could not include multiple
commands), so it maybe made sense to have different retry behaviors.

Now, execute and query are basically identical except for whether data
is returned, and so I don't think there is any reason why they
shouldn't both do retries.

(I modeled the implementation after how I did this in the edgedb test
suite's hacked up client: edgedb/edgedb#8249)

@msullivan msullivan requested review from 1st1, elprans and fantix February 13, 2025 18:57
In the distant past, execute was quite different than query (execute
couldn't take arguments and query could not include multiple
commands), so it maybe made sense to have different retry behaviors.

Now, execute and query are basically identical except for whether data
is returned, and so I don't think there is any reason why they
shouldn't both do retries.

(I modeled the implementation after how I did this in the edgedb test
suite's hacked up client: edgedb/edgedb#8249)
Copy link
Member

@fantix fantix left a comment

Choose a reason for hiding this comment

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

LGTM!

@msullivan msullivan merged commit e263f2b into master Feb 13, 2025
62 checks passed
@msullivan msullivan deleted the retry-execute branch February 13, 2025 21:58
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.

2 participants