Marten: Retry when transient errors occur

[cocowalla]

Is there anything built in to Marten to enable automatic retries when transient errors occur, or some kind of extensibility point where it could be plugged in?

[mysticmind]

@cocowalla There isn't any built-in facility I am aware of within Marten. I have had good success using https://github.com/App-vNext/Polly in my own projects dealing with retry/circuit breaker logic (albeit haven't applied to a Marten based project as yet). You can roll your own using Polly or contribute back as a separate library to use with Marten :-)

[mysticmind]

Did a quick search, this blog post could be useful starting point...

[jokokko]

@mysticmind I think this is something that sample code could be composed for under "Scenarios" in the docs http://jasperfx.github.io/marten/documentation/scenarios/

[mysticmind]

Interesting issue thread and relevant to the issue in question https://github.com/npgsql/npgsql/issues/1495. NpgsqlException exposes IsTransient so any retry logic should be based on this.

[cocowalla]

@mysticmind Yes, I had noticed the IsTransient property - very useful! I'm also familiar with Polly, but I don't really want to pepper my code with retries everywhere, and would much prefer to handle it at more of an infrastrutcure level. The codebase I'm working with happens to be quite amenable to that, but I was wondering if this is something that Marten supported for more general use.

If I wanted to add this to Marten, where do you think it should go? Perhaps an extended version of ManagedConnection?

[mysticmind]

@cocowalla yes, it will need to be an extended version of ManagedConnection since it handles NpgsqlException. Also I am thinking about a mechanism via which this could be implemented without Marten having dependency on Polly. Thoughts?

@jeremydmiller @jokokko @oskardudycz Thoughts? Or is it okay for Marten to have Polly as a dependency?

[cocowalla]

@mysticmind I hadn't looked too much into Marten internals, and I'd just assumed that it would have something baked in to customise dependency resolution, such that things could be easily switched out for custom implementations. I don't see anything like that though - I'll open another issue for discussion 😊

In lieu of that, I have a couple of ideas:

1. Add a bool EnableRetries property to StoreOptions to allow enabling automatic retries, and add a new RetryingManagedConnection class that extends ManagedConnection, adding retries using Polly (so this does mean taking a dependency on Polly)

2. Add a Func<IManagedConnection> property to StoreOptions so that _any_ IManagedConnection can be used in DocumentStore. We could then have a separate Marten.Polly library that has a RetryingManagedConnection implementation (so this means no dependency on Polly from the core Marten library)

Thoughts?

[jeremydmiller]

I'm going to veto taking a direct dependency on Polly in the main Marten library because that's a recipe for diamond dependency hell and it'll lead to folks having all kinds of Nuget problems. Having some kind of plug in that can build the IManagedConnection and do it from an add on, maybe. Makes the ManagedConnection awfully heavyweight, but it already was to begin with.

[jokokko]

Overall, with the retry logic I'd be afraid of building a framework within framework. So having code samples on how to use Marten with Polly and then Marten.Polly library if needed would sound like the best way to go (imho).

[cocowalla]

@jokokko So first you create a policy:

var retryPolicy = Polly.Policy
    .Handle<NpgsqlException>(ex => ex.IsTransient)
    .Or<TimeoutException>()
    .Or<SocketException>()
    .WaitAndRetryAsync(6, StandoffFunc, (ex, timespan, retryAttempt, ctx) =>
    {
        // Logging here
    });

Then you need to wrap every call to SaveChanges/SaveChangesAsync/Patch like:

await retryPolicy.ExecuteAsync(async () => await session.SaveChangesAsync());

So it's very simple - but it's a pain to have to remember to do this every time, so it really needs to be centralised somewhere, and that's the problem here - I don't see anywhere within Marten to put it.

That leaves users being forced to add a facade over Marten's ISession, which I'd personally rather not do; ISession already provides a nice abstraction, essentially a generic repository. So it would be nice to add an extension point as discussed here, or for someone more familiar with the codebase to show how it could better be done.

[mysticmind]

Possibly a different approach as below:

• Marten exposes IRetryPolicy interface as an extension point

public interface IRetryPolicy
{
    void Execute(Action operation);

    TResult Execute<TResult>(Func<TResult> operation);

    Task Execute(Func<Task> operation, CancellationToken cancellationToken);

    Task<TResult> Execute<TResult>(Func<Task<TResult>> operation, CancellationToken cancellationToken);
}

• Consumers can implement this interface and pass an instance to configure DocumentStore. This enables the ability to create a library based on Polly or something else.
• Calls within ManagedConnection can wrap the calls using the IRetryPolicy methods.

Thoughts?

[cocowalla]

Yep, that works for me too. If this (or an evolution of it) gets the go-ahead and someone from the core team does the Marten side of things, I'm happy to have a stab at a library with a Polly implementation?

[mysticmind]

@jeremydmiller @jokokko @cocowalla I have implemented the IRetryPolicy in a feature branch in my forked repo (you can see the reference comment above). Please take a look and provide your comments/feedback.

Edit: I went ahead and created a PR #1124 for you to review