-
Notifications
You must be signed in to change notification settings - Fork 210
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
fix: add exponential backoff to autocommit with random delay #4015
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4015 +/- ##
==========================================
- Coverage 57.65% 57.61% -0.04%
==========================================
Files 193 193
Lines 42883 42883
==========================================
- Hits 24723 24706 -17
- Misses 18160 18177 +17 ☔ View full report in Codecov by Sentry. |
039e20a
to
861e871
Compare
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.
Left some nits, but should improve performance in concurrent settings, so ACKing.
@@ -455,6 +457,9 @@ impl Database { | |||
} | |||
} | |||
} | |||
let delay = (2u64.pow(curr_attempts.min(7) as u32) * 10).min(1000); |
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.
The second min
seems a bit redundant given that the max delay would still be 1280ms without it.
let delay = (2u64.pow(curr_attempts.min(7) as u32) * 10).min(1000); | ||
let delay = rand::thread_rng().gen_range(delay..(2 * delay)); |
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.
nit: delay
doesn't have a semantic type, better alternatives would be:
- Converting to
Duration
as soon as possible (idk if we can generate a range ofDuration
, but worth a try) - Give a more descriptive name that includes the unit (ms)
this makes transactions less likely to conflict in next try
originally part of #3989