Skip to content
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

Merged
merged 1 commit into from
Jun 21, 2021
Merged

Improve DTC check #860

merged 1 commit into from
Jun 21, 2021

Conversation

DavidBoike
Copy link
Member

@DavidBoike DavidBoike commented Jun 17, 2021

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.

@DavidBoike DavidBoike added this to the 6.3.0 milestone Jun 17, 2021
@DavidBoike DavidBoike requested review from kbaley and kentdr June 17, 2021 20:36
@DavidBoike DavidBoike self-assigned this Jun 17, 2021
@@ -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`. " +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"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?

Copy link
Member Author

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.

Copy link
Member Author

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.

@DavidBoike DavidBoike merged commit 615ab36 into vnext Jun 21, 2021
@DavidBoike DavidBoike deleted the better-dtc-check-6-3 branch June 21, 2021 14:26
@DavidBoike DavidBoike changed the title Better DTC check, doesn't even open connection on netcore (6.3) Improve DTC check Jun 23, 2021
@MarcWils
Copy link

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?

@SzymonPobiega
Copy link
Member

SzymonPobiega commented Oct 27, 2021

@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:

@DavidBoike
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants