-
Notifications
You must be signed in to change notification settings - Fork 96
Automatically retry busy errors when creating write transactions #945
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
base: main
Are you sure you want to change the base?
Automatically retry busy errors when creating write transactions #945
Conversation
0f5adc0 to
83df783
Compare
83df783 to
81d72c7
Compare
|
|
||
| informerObjectCachePerms fs.FileMode = 0o600 | ||
|
|
||
| maxBeginTXAttemptsOnBusyErrors = 3 |
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.
Should we expose this value via config?
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.
Hmm I have mixed feelings, it's probably something we will never need to tweak.
The error we are trying to mitigate is very specific and unlikely to happen, but still possible, and the only way to deal with it is to retry.
If it happens more than 3 times in a row it may be a sign of a bigger problem and we may not want to allow to ignore that?
ericpromislow
left a comment
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.
Looks fine with the suggestion
81d72c7 to
8bdc4b5
Compare
Issue: rancher/rancher#52872
Detect
SQLITE_BUSYandSQLITE_BUSY_SNAPSHOTwhich can happen normally on a concurrent system using WAL mode. Retry them automatically, since that specific case won't be managed by the busy handler.Note:
BeginTxreturns a*sql.Txwhich does not expose any public field, so I have to introduce a new interface in order to be able to mock it for my unit tests. Other kind of testing would be challenging since this patch is meant to mitigate a possible race condition.