-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Support for SqlRetryLogic #1864
Comments
My limited understanding (from here is that |
Is there a plan to support it? |
Honestly, today is the first time I've heard of it. Typically, we prefer to target the primary ADO.NET API surface (since we don't want to force any provider-specific dependency trees), and this is very specifically a Microsoft.Data.SqlClient feature, which makes it a little awkward to support. We'd need to think about it, basically. |
It would be advantageous to include a retry on fail mechanism in the built-in methods, eliminating the need to create wrappers for all Dapper's methods to handle the errors. Currently, I am doing something like this: public new IEnumerable<T> Query<T>(string sql, object param = null)
{
var retries = 0;
while (true)
{
try
{
// Try to execute the query
return connection.Query<T>(sql, param); //connection is IDbConnection
}
catch (Exception ex)
{
// Check if the exception is a network error (replace ORA-12570 with the actual error code)
if (ex.Number == 'ORA-12570' ) //a list of transient errors
{
if (++retries == _retryCount)
{
throw; // If all retries failed, propagate the exception.
}
Thread.Sleep(TimeSpan.FromSeconds(Math.Pow(2, retries))); // Wait before retrying.
}
else
{
throw; // If it's not a network error, propagate the exception.
}
}
}
}
``` |
As you can see from even your own example, there isn't a good way to know when to bubble up and when to retry, then we have a max retryCount to content with. A DbConcurrencyException will need additional cleanup code before retrying, a SqlException with message of timeout is another behavior. You should be wrapping all Dapper calls with your own layer nevertheless. |
That's true. public class RetryOnErrorStrategyForOracle : OracleRetryingExecutionStrategy // in Dapper
{
private static readonly IList<int> ErrorNumbersToRetry = new List<int>()
{
12570, 12571, 03135 //lost connection errors
};
public SomeRetryStrategy()
: base(DefaultMaxRetryCount, ErrorNumbersToRetry)
{
}
}
//configure Dapper to use the custom strategy in the methods
connection.Query("select * from ...", new RetryOnErrorStrategyForOracle()) Now Dapper could use the configured retry strategy. To keep it simple, it could just use the sample error code list provided here for the OracleRetryingExecutionStrategy implementation. This would help retry when connection errors occur - I often run into errors 12570, 12571, 03135 when the connection is lost. OK, Wrapping all the Dapper methods to handle errors could work as a workaround, but modifying Dapper itself to support configurable retry strategies would be a more elegant solution. Thanks! |
I don't want to step on @mgravell and his thoughts for this library. Dapper should do Dapper things, if we want retry features: errors, exceptions, we should extend Dapper with a nuget package of
|
I agree with @billrob's comments above; additionally, a low effort implementation is probably more harmful than helpful. If we you transient retry and resilience - that sounds like a job for Polly, no? Let Dapper do what it knows, and let Polly do what it knows. IMO. |
I understand. Thanks for providing that insight |
Came across this issue while trying to find a way to use SqlRetryLogic. Support for the ability to use the out-of-the-box retries provided by Microsoft.Data.SqlClient would be a big win for me and my team over having to write our own wrapper around dapper and being forced to define the retry policy logic instead of just using the microsoft provided default. @mgravell For example, a hypothetical var cmd = cnn.CreateCommand();
configureCommand?.Invoke(cnn, cmd);
var init = GetInit(cmd.getType());
... rest of code ... |
@RyanThomas73 Doesn't the SqlConnection object allows this already?
|
Yes the connection does, but as mentioned in the above that only helps with connection.Open() call. My request is for an extension point to configure the For example, if the Action<IDbConnection, IDbCommand> configureCommand = (dbConnection, dbCommand) => {
((SqlCommand)dbCommand).RetryLogicProvider = ((SqlConnection)dbConnection).RetryLogicProvider;
} Such an extension point would |
Yes, you are correct. I would have predicted it would have used the retry logic on the connection if it wasn't overwritten on the command. I can see their point for not doing it. Although I propose to @mgravell
It looks like all code paths end up on
so it would just be some monkey work to add the optional parameter in everywhere. Would that be more useful? I suppose the only confusing might be if the caller sets their own command.xxx properties and Dapper's internal implementation changes some of that. That would be frustrating and confusing. |
The SqlMapper extension methods already have overloads that take the Since this is a fairly advanced use case I would argue that it would be sufficient to add such a Consumers that need to leverage this extension point could then create the CommandDefinition directly. e.g.
|
DapperAOT already has something like this - [CommandFactory] - any use? https://github.com/DapperLib/DapperAOT/blob/0b5f6d7827d74632c1b239d6ceb9f9f4ea24e8d5/test/Dapper.AOT.Test/Interceptors/BaseCommandFactory.input.cs#L20 |
…andFactory function - Issue DapperLib#1864
I put together a commit with a minimal solution using just a simple function for the command factory, if that helps any: I also took a look at the CommandFactory Marc mentioned from DapperAOT. The additional extension points beyond just command creation - initialization of the command text and command type, parameter addition, and optional post processing look like they would add lots of customizability but also seem like they would be challenging to retrofit onto the existing codebase/public api for Dapper |
Hi @RyanThomas73 I got distracted on working on this because I was getting the docker containers built up to run all the provider tests. I actually have two approaches I was going to @mgravell with. This one ctor change is more risky because of the optional parameter with I also have another branch that has a I'll defer to Marc here. Also as note, you put an if check and directly invoke. The performance different is small but still is an order of magnitude slower.
|
@RyanThomas73 I went with a new constructor overload to minimize impact across the optional parameters. |
@billrob Awesome! Your PR definitely solves my use cases. Side Note: Not sure if the dapper maintainers have any interested in this approach - I have a Roslyn analyzer + code fix provider you can see here that flags the use of optional parameters and recommends/fixes them to use explicit |
@billrob If it's not a huge burden could you share a gist of your microbenchmark for this? |
To others that want to do this you can create a proxy IDbConnection then run Dapper against that: private class RetryLogicProviderProxy(SqlConnection conn, SqlRetryLogicBaseProvider retryLogicProvider ) : IDbConnection
{
public void Open() => conn.Open();
[AllowNull]
public string ConnectionString
{
get => conn.ConnectionString;
set => conn.ConnectionString = value;
}
public int ConnectionTimeout => conn.ConnectionTimeout;
public string Database => conn.Database;
public ConnectionState State => conn.State;
public IDbCommand CreateCommand()
{
var cmd = conn.CreateCommand();
cmd.RetryLogicProvider = retryLogicProvider;
return cmd;
}
// ..snip..
} Usage: var retryLogicProvider = SqlConfigurableRetryFactory.CreateExponentialRetryProvider(new SqlRetryLogicOption()
{
MinTimeInterval = TimeSpan.FromSeconds(2),
MaxTimeInterval = TimeSpan.FromSeconds(10),
DeltaTime = TimeSpan.FromSeconds(2),
NumberOfTries = 5,
TransientErrors = [208] // Table not found
});
using SqlConnection inner = new SqlConnection(connectionString) { RetryLogicProvider = retryLogicProvider };
using IDbConnection db = new RetryLogicProviderCommandProxy(inner, retryLogicProvider); // Could use a different provider for commands.
db.Open();
//
const string sql = "SELECT * FROM [DoesNotExist]";
db.ExecuteReader(sql); // Will retry |
I experimented with doing this and I ran into issues that made me think SqlConnection has internal logic tied to it's specific type that doing this will break. e.g. Try using the above while enlisting in an ambient transaction via transaction scope.
Additional Notes: I think I was using DbConnection and async signatures as well with the issues I saw compared to your IDbConnection and db.Open() example. |
Hello,
Is there a plan to support the new SqlRetryLogic in dapper?
A way to pass the retry logic to the SqlCommand object?
Thanks
The text was updated successfully, but these errors were encountered: