-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add retry for BeginTransaction #183
Conversation
@@ -89,7 +89,7 @@ public async Task OpenAsync(IsolationLevel isolationLevel) | |||
|
|||
// We use the synchronous overload here even though there is an async one, because the BeginTransactionAsync calls |
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.
Doesn't seem like a async overload will be very helpful.
@@ -45,7 +45,7 @@ | |||
<PackageReference Include="Newtonsoft.Json" Version="12.0.3" /> | |||
<PackageReference Include="System.Diagnostics.Contracts" Version="4.3.0" /> | |||
<PackageReference Include="System.ValueTuple" Version="4.4.0" /> | |||
<PackageReference Include="Microsoft.Data.SqlClient" Version="2.1.4" /> | |||
<PackageReference Include="Microsoft.Data.SqlClient" Version="4.1.0" /> |
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'd recommend against using the sqlClient retry provider for now until we have a much more stable build environment. Currently, I'm only aware of this option for opening connections (not beginning transactions, but this advice applies to both in case that comes to light). Perhaps we don't want to make that change at all regardless.
6cc0ac8
to
61aa573
Compare
@@ -14,7 +14,7 @@ public IntegrationTestDatabase() | |||
var username = Environment.GetEnvironmentVariable("NevermoreTestUsername"); | |||
var password = Environment.GetEnvironmentVariable("NevermoreTestPassword"); | |||
testDatabaseName = Environment.GetEnvironmentVariable("NevermoreTestDatabase") ?? "Nevermore-IntegrationTests"; | |||
var builder = new SqlConnectionStringBuilder($"Server={sqlInstance};Database={testDatabaseName};{(username == null ? "Trusted_connection=true;" : string.Empty)}") | |||
var builder = new SqlConnectionStringBuilder($"Server={sqlInstance};Database={testDatabaseName};{(username == null ? "Trusted_connection=true;" : string.Empty)}; Encrypt=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.
Bumping the SqlClient altered the default for encryption. Most devs probably don't have this enabled on their dev SQL Server instance, so we can set this to avoid forcing folks to have to set another env var, or change their server settings.
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.
Will we need to add this on all connection strings that we have on Octopus?
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.
Yeah possibly / probably 👍 - good call
61aa573
to
0c6b31e
Compare
@@ -13,7 +13,7 @@ public IntegrationTestDatabase() | |||
var username = Environment.GetEnvironmentVariable("NevermoreTestUsername"); | |||
var password = Environment.GetEnvironmentVariable("NevermoreTestPassword"); | |||
testDatabaseName = Environment.GetEnvironmentVariable("NevermoreBenchmarkDatabase") ?? "Nevermore-Benchmarks"; | |||
var builder = new SqlConnectionStringBuilder($"Server={sqlInstance};Database={testDatabaseName};{(username == null ? "Trusted_connection=true;" : string.Empty)}") | |||
var builder = new SqlConnectionStringBuilder($"Server={sqlInstance};Database={testDatabaseName};{(username == null ? "Trusted_connection=true;" : string.Empty)}; Encrypt=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.
See below
0c6b31e
to
14bb02d
Compare
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 left some comments in there. Thanks Paul!
@@ -14,7 +14,7 @@ public IntegrationTestDatabase() | |||
var username = Environment.GetEnvironmentVariable("NevermoreTestUsername"); | |||
var password = Environment.GetEnvironmentVariable("NevermoreTestPassword"); | |||
testDatabaseName = Environment.GetEnvironmentVariable("NevermoreTestDatabase") ?? "Nevermore-IntegrationTests"; | |||
var builder = new SqlConnectionStringBuilder($"Server={sqlInstance};Database={testDatabaseName};{(username == null ? "Trusted_connection=true;" : string.Empty)}") | |||
var builder = new SqlConnectionStringBuilder($"Server={sqlInstance};Database={testDatabaseName};{(username == null ? "Trusted_connection=true;" : string.Empty)}; Encrypt=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.
Will we need to add this on all connection strings that we have on Octopus?
|
||
public static DbTransaction BeginTransactionWithRetry(this SqlConnection connection, IsolationLevel isolationLevel, string sqlServerTransactionName, RetryPolicy retryPolicy) | ||
{ | ||
return (retryPolicy ?? RetryPolicy.NoRetry).LoggingRetries("Beginning Database Transaction").ExecuteAction(() => connection.BeginTransaction()); |
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.
return (retryPolicy ?? RetryPolicy.NoRetry).LoggingRetries("Beginning Database Transaction").ExecuteAction(() => connection.BeginTransaction()); | |
return (retryPolicy ?? RetryPolicy.NoRetry).LoggingRetries("Beginning Database Transaction").ExecuteAction(() => connection.BeginTransaction(isolationLevel, sqlServerTransactionName)); |
Don't we need to continue passing those arguments?
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.
🤦 Yes, haha of course. Thanks for catching this :D
@akirayamamoto We might have to set it to |
Octopus Deploy dev teams have noticed transient connectivity issues when calling
BeginTransaction
inReacTransaction.cs
.We are also behind 2 versions of SqlClient that may have beneficial bug fixes or performance improvements..
The objective of this PR is to:
Slack Thread
Benchmark Results
Before
TODO: get these running (currently hitting errors)
After
TODO: Same