-
Notifications
You must be signed in to change notification settings - Fork 321
Expose copy default transient error list #3903
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?
Expose copy default transient error list #3903
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.
Pull request overview
This PR exposes the previously private default transient error list as a public property to address issue #2342, allowing consumers to extend the error code list without needing to copy/paste the base set from the repository.
Changes:
- Adds a new public property
DefaultTransientErrorstoSqlConfigurableRetryFactorythat returns a read-only copy of the default transient error list - Removes duplicated error list from test helper class
RetryLogicTestHelperand migrates to use the new public API - Adds XML documentation for the new public property and documents additional error codes (64, 20, 0, -2, 207, 18456, 42108, 42109)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Reliability/SqlConfigurableRetryFactory.cs | Adds public DefaultTransientErrors property that returns a ReadOnlyCollection<int> copy of the private transient error list; adds missing error codes to the private list |
| src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/RetryLogic/RetryLogicTestHelper.cs | Removes duplicated private error list and updates code to reference the new public SqlConfigurableRetryFactory.DefaultTransientErrors property |
| doc/snippets/Microsoft.Data.SqlClient/SqlConfigurableRetryFactory.xml | Adds XML documentation for the new property and documents the previously undocumented error codes |
...osoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Reliability/SqlConfigurableRetryFactory.cs
Outdated
Show resolved
Hide resolved
...osoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Reliability/SqlConfigurableRetryFactory.cs
Outdated
Show resolved
Hide resolved
...osoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Reliability/SqlConfigurableRetryFactory.cs
Outdated
Show resolved
Hide resolved
@dotnet-policy-service agree |
...osoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Reliability/SqlConfigurableRetryFactory.cs
Outdated
Show resolved
Hide resolved
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
paulmedynski
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.
Thanks for the submission. The team will need to discuss the public API changes, and I have one implementation suggestion.
| }; | ||
| 233, // A connection was successfully established with the server, but then an error occurred during the login process. (provider: Shared Memory Provider, error: 0 - No process is on the other end of the pipe.) (Microsoft SQL Server, Error: 233) | ||
| 64, // A connection was successfully established with the server, but then an error occurred during the login process. (provider: TCP Provider, error: 0 - The specified network name is no longer available.) | ||
| 20, // The instance of SQL Server you attempted to connect to does not support encryption. |
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'm not sure how this error (or 207 or 18456) are transient. I'll discuss with the team.
| 997, // A connection was successfully established with the server, but then an error occurred during the login process. (provider: Named Pipes Provider, error: 0 - Overlapped I/O operation is in progress) | ||
| 233 // A connection was successfully established with the server, but then an error occurred during the login process. (provider: Shared Memory Provider, error: 0 - No process is on the other end of the pipe.) (Microsoft SQL Server, Error: 233) | ||
| }; | ||
| 233, // A connection was successfully established with the server, but then an error occurred during the login process. (provider: Shared Memory Provider, error: 0 - No process is on the other end of the pipe.) (Microsoft SQL Server, Error: 233) |
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.
These changes constitute a public API change (altering which errors we consider transient by default), so this will need some extra scrutiny.
...osoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Reliability/SqlConfigurableRetryFactory.cs
Outdated
Show resolved
Hide resolved
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.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
...osoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Reliability/SqlConfigurableRetryFactory.cs
Outdated
Show resolved
Hide resolved
doc/snippets/Microsoft.Data.SqlClient/SqlConfigurableRetryFactory.xml
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netfx/ref/Microsoft.Data.SqlClient.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/ref/Microsoft.Data.SqlClient.cs
Outdated
Show resolved
Hide resolved
...osoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Reliability/SqlConfigurableRetryFactory.cs
Outdated
Show resolved
Hide resolved
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
...osoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Reliability/SqlConfigurableRetryFactory.cs
Outdated
Show resolved
Hide resolved
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.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
doc/snippets/Microsoft.Data.SqlClient/SqlConfigurableRetryFactory.xml
Outdated
Show resolved
Hide resolved
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
@David-Engel Since this changes our public API, could you give this a look to see if it's something we should take? |
benrr101
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.
I like it and don't see any issues - just want to get some PM approval on it before we add to our public API!
|
#649 also discusses transient errors. |
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #3903 +/- ##
==========================================
- Coverage 76.66% 69.45% -7.22%
==========================================
Files 269 263 -6
Lines 43246 66178 +22932
==========================================
+ Hits 33156 45962 +12806
- Misses 10090 20216 +10126
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
My main hesitancy is I don't want changes to the list to be considered breaking changes. We already have enough things that require us to major vbump. If we can expose them in a way that changes to the error list is not considered breaking, I'm all for it. |
|
@paulmedynski So how should we proceed? Are these considered breaking? I've only added the ones that were already used in the tests. |
|
Hi @MatthiasHuygelen - The team will be meeting to discuss on Thursday, and I'll post an update after that. |
| -2, // Execution Timeout Expired. The timeout period elapsed prior to completion of the operation or the server is not responding. | ||
| 0, // A network-related or instance-specific error occurred while establishing a connection to SQL Server. The server was not found or was not accessible. Verify that the instance name is correct and that SQL Server is configured to allow remote connections. (provider: TCP Provider, error: 0 - A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond.) | ||
| 20, // The instance of SQL Server you attempted to connect to does not support encryption. | ||
| 64, // A connection was successfully established with the server, but then an error occurred during the login process. (provider: TCP Provider, error: 0 - The specified network name is no longer available.) | ||
| 207, // Invalid column name |
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.
Why are we adding these error codes to transient list? Are they documented somewhere as transient?
If not, they should be removed.
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.
If these shouldn't be in the list why were they added to the transient error list of your tests ? RetryLogicTestHelper.cs
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.
That is likely done to mock a failure in tests, where its less important to test a 'transient' error code but more important to test whether the retry logic works as expected when a particular error occurs. Also our tests generate these error codes due to a particular configuration in use, which is not a generic condition for those errors to occur.
Publicly documented transient errors are the only ones we should be using as default errors SqlClient would retry on.
cheenamalhotra
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.
Do not add new error codes as transient without documentation proofs.
| 10060, // An error has occurred while establishing a connection to the server. When connecting to SQL Server, this failure may be caused by the fact that under the default settings SQL Server does not allow remote connections. (provider: TCP Provider, error: 0 - A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond.) (Microsoft SQL Server, Error: 10060) | ||
| 10928, // Resource ID: %d. The %s limit for the database is %d and has been reached. For more information, see http://go.microsoft.com/fwlink/?LinkId=267637. | ||
| 10929, // Resource ID: %d. The %s minimum guarantee is %d, maximum limit is %d and the current usage for the database is %d. However, the server is currently too busy to support requests greater than %d for this database. For more information, see http://go.microsoft.com/fwlink/?LinkId=267637. Otherwise, please try again later. | ||
| 18456, // Using managed identity in Azure Sql Server throws 18456 for non-existent database instead of 4060. |
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.
18456 : This is also not a transient error, in tests it's a different usecase where retrying makes sense, but it's not a retriable error in most cases.
Description
The default set of transient error codes is private. This seems to make it difficult to extend error codes without copy/pasting the base set from this repository. So we expose a copy.
Issues
#2342