-
Notifications
You must be signed in to change notification settings - Fork 114
feat: add retries to sql macros #2529
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: add retries to sql macros #2529
Conversation
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.
PR Summary
Adds SQL query retry mechanism with exponential backoff and improves memory efficiency across the codebase.
- Added retry logic in
sql_query_macros.rs
with 100ms base retry and max 16 retries - Removed
MaxSqlRetries
error variant fromWorkflowError
in favor of macro-level retry handling - Renamed
query
totxn
in database drivers for better semantic clarity - Added reference operators (
&
) to SQL query parameters across multiple services to prevent unnecessary cloning - Simplified transaction retry logic to use consistent backoff for all database errors
12 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile
if is_retrying.load(Ordering::Relaxed) { | ||
self.query(|| async { | ||
sql_execute!( | ||
[self, &pool] | ||
" | ||
DELETE FROM workflow_signal_events | ||
WHERE location = jsonb(?1) | ||
", | ||
location, | ||
) | ||
.await | ||
}) | ||
sql_execute!( | ||
[self, &pool] | ||
" | ||
DELETE FROM workflow_signal_events | ||
WHERE location = jsonb(?1) | ||
", | ||
location, | ||
) | ||
.await | ||
.map_err(|x| fdb::FdbBindingError::CustomError(x.into()))?; | ||
} |
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.
logic: Potential race condition: the DELETE operation should be in the same transaction as the INSERT to maintain atomicity
let mut backoff = $crate::__rivet_util::Backoff::new( | ||
4, | ||
None, | ||
$crate::utils::sql_query_macros::QUERY_RETRY_MS, | ||
50 | ||
); |
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.
logic: Backoff parameters are inconsistent - base=4 and factor=50 here vs QUERY_RETRY_MS=100 defined at the top. Consider aligning these values or documenting why they differ.
Database(_) | Io(_) | Tls(_) | Protocol(_) | PoolTimedOut | PoolClosed | ||
| WorkerCrashed => { |
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.
style: Consider adding RowNotFound to non-retryable errors since retrying won't help if the row doesn't exist
Database(_) | Io(_) | Tls(_) | Protocol(_) | PoolTimedOut | PoolClosed | |
| WorkerCrashed => { | |
Database(_) | Io(_) | Tls(_) | Protocol(_) | PoolTimedOut | PoolClosed | |
| WorkerCrashed | RowNotFound => { |
Deploying rivet with
|
Latest commit: |
c307626
|
Status: | ✅ Deploy successful! |
Preview URL: | https://758a1d78.rivet.pages.dev |
Branch Preview URL: | https://06-03-feat-add-retries-to-sq.rivet.pages.dev |
200c987
to
b3eae4a
Compare
Deploying rivet-studio with
|
Latest commit: |
c307626
|
Status: | ✅ Deploy successful! |
Preview URL: | https://bb342d1e.rivet-studio.pages.dev |
Branch Preview URL: | https://06-03-feat-add-retries-to-sq.rivet-studio.pages.dev |
Deploying rivet-hub with
|
Latest commit: |
c307626
|
Status: | ✅ Deploy successful! |
Preview URL: | https://6498ef2a.rivet-hub-7jb.pages.dev |
Branch Preview URL: | https://06-03-feat-add-retries-to-sq.rivet-hub-7jb.pages.dev |
5e33d41
to
7666132
Compare
b3eae4a
to
4fc5f51
Compare
4fc5f51
to
406d6f9
Compare
7666132
to
20eace2
Compare
20eace2
to
b77a826
Compare
406d6f9
to
d44c0b9
Compare
d44c0b9
to
c307626
Compare
b77a826
to
863f139
Compare
Merge activity
|
<!-- Please make sure there is an issue that this PR is correlated to. --> ## Changes <!-- If there are frontend changes, please include screenshots. -->
Changes