-
Notifications
You must be signed in to change notification settings - Fork 275
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
Implement SqlException.IsTransient #2127
base: main
Are you sure you want to change the base?
Conversation
} | ||
} | ||
|
||
return false; |
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.
What happens if there's both a transient and a non-transient error? This would return IsTransient=true, which doesn't seem right.
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.
You think it is not? In which cases there are multiple errors returned for an exception? Anyway, if we retry because there is at least one transient error, and that error disappear, we can perhaps expect that other non-transient error will disappear. If we fail early we will not be able to detect if they are related.
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.
Anyway, if we retry because there is at least one transient error, and that error disappear, we can perhaps expect that other non-transient error will disappear. If we fail early we will not be able to detect if they are related.
I'm not very knowledgeable about SQL Server/SqlClient error handling (or which situation produces multiple errors), but generally speaking, the whole point of an error being non-transient is that it doesn't just go away when you retry the same operation. So I don't think it makes sense for IsTransient to return true when there's at least one non-transient error.
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.
I have inverted the check. The code will return true only if all errors are transient. It will return false on first non-transient (or no errors)
Ping @AndriySvyryd which has handled most of the work on dotnet/efcore. |
There already is a list of transient errors in the codebase, let's stick to this one: Line 40 in c7cefee
|
based EFCore SqlServerTransientExceptionDetector: https://github.com/dotnet/efcore/blob/main/src/EFCore.SqlServer/Storage/Internal/SqlServerTransientExceptionDetector.cs
78d5f44
to
85ae0c5
Compare
What could be the prefered way to reuse that list? Add an internal method that I could call from the property or move out the list of codes in a specific class? |
future commit will include a full table of known transient error codes
Co-authored-by: Luis Orjuela <136631445+log-trader@users.noreply.github.com>
Ping @ErikEJ to resurect this PR. |
It would be great to progress this PR if possible -- it's really frustrating as a developer to find a field named My 2c would be that the list of errors that @ErikEJ referred to is considerably smaller than the list of errors handled by the EF code, which makes me wonder how the list in It would be good to get some feedback from some of the frequent contributors to confirm the best course of action to get this change merged and make the |
Just to add to this, the EF code has been updated very recently, while the SqlClient list is 3 years old. Seems much better to go with the former. Importantly, the most recent addition to the EF switch statement does a check on more than just the number, so a HashSet of ints isn't going to cut it really. @tbolon probably worth updating this PR to include the latest changes referred to above :) |
Fix #649
Code is based (it's a simple copy/paste for now) on EFCore SqlServerTransientExceptionDetector.cs
The xmldoc is quite empty (and does not respect the convention), I expect some help and/or guidance about how it should be handled: should we document all the cases handled by the code? Or keep it as a black box? Or something between where the a direct link to the code is added in the documentation?
Should we move the code outside the class to keep the file size small?
Should we expose this algorithm outside (make the external class public) so other libraries can use it?