-
Notifications
You must be signed in to change notification settings - Fork 36
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
Improve DTC check #860
Improve DTC check #860
Conversation
@@ -281,13 +287,13 @@ async Task<StartupCheckResult> TryEscalateToDistributedTransactions(TransactionO | |||
"Note that different transaction modes may affect consistency guarantees as you can't rely on distributed " + | |||
"transactions to atomically update the database and consume a message. Original error message: " + ex.Message; | |||
} | |||
catch (SqlException sqlException) | |||
catch (Exception exception) | |||
{ | |||
message = "Could not escalate to a distributed transaction while configured to use TransactionScope. Check original error message for details. " + | |||
"In case the problem is related to distributed transactions you can still use SQL Server transport but " + | |||
"should specify a different transaction mode via `EndpointConfiguration.UseTransport<SqlServerTransport>().Transactions`. " + |
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 specify a different transaction mode via `EndpointConfiguration.UseTransport<SqlServerTransport>().Transactions`. " + | |
"should specify a different transaction mode by setting the `TransportTransactionMode` property on the `SqlServerTransport` instance when configuring the endpoint. " + |
WIth the 8.0 API changes UseTransport()
returns the routing settings so the transaction mode has to be set directly on the transport instance. The above sentence a bit less specific than it was prior and possibly confusing. Maybe we should send them to the docs instead?
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.
We don't link in docs from components because we have no good way to ensure those links stay valid. But this is the 6.3 version so the UseTransport form is correct.
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.
But I am going to steal your wording (it's a bit hand-wavy but it's accurate) for the OTHER pull request on master where the message was not updated by the transport seam TF.
We run a deployment scenario with Azure SQL and elastic transactions. Which I believe is a common scenario. These changes causes the DTC to (falsely) fail. Is avoiding one extra SQL connection really worth triggering these possible false warnings? |
@MarcWils thanks for your feedback. I guess we did not take elastic transactions into account. I'll add this to the scope of the next enhancement release. I raised the following issue to track it: |
Worth noting that when we "simplified" that, it's because it was causing some issues in the automated tests. That could have been due to Microsoft.Data.SqlClient 3.0.x which we found out has a DTC-related bug, but unfortunately I don't remember more details than that. |
When configured for distributed transactions, a SQL Transport endpoint will attempt to determine if the platform (.NET Framework or .NET Core) and the target SQL Server both support distributed transactions. By using a fake resource manager, in .NET Core no connections to the target SQL Server need to be made to determine that the platform does not support DTC. On .NET Framework, one connection must still be made, but by using the fake resource manager only one (rather than two) are required.