Overhaul of the execution pipeline #2918
Conversation
jeremydmiller
commented
Jan 19, 2024
jeremydmiller
commented
- New "auto-closing connection lifetime" by default
- Async daemon is using NpgsqlBatch
- Ripped out IRetryPolicy
- Getting the operation processing ready for effective retries
* New "auto-closing connection lifetime" by default * Async daemon is using NpgsqlBatch * Ripped out IRetryPolicy * Getting the operation processing ready for effective retries
…e to make Polly application easier
| <!-- endSnippet --> | ||
|
|
||
| Also you could use the fantastic [Polly](https://www.nuget.org/packages/polly) library to easily build more resilient and expressive retry policies by implementing `IRetryPolicy`. | ||
| Marten's previous, homegrown `IRetryPolicy` mechanism was completely replaced by [Polly](https://www.nuget.org/packages/polly) in Marten V7. |
There was a problem hiding this comment.
SUGGESTION: It'd be good to provide some samples on how to plug Polly policies to Marten.
There was a problem hiding this comment.
Yeah, I agree. Like I told y'all in Discord, I hadn't made any effort to document anything yet.
|
The PR scope and type of changes is big, so pardon me if I didn't get it right. I assume that the only way to set a reusing connection now is to either:
Is it correct? I'd love to get rid of those injections with the Connection and lazy-loaded stuff, but I'm not sure if that won't be too big a change for some of the users and would make them stay on the older versions. On the other hand, that's more of a guess for now on how many people are using such a way. We still need to do benchmarks if there's no performance degradation, as the opening connection can be quite costly, especially around the non-pooled usage. I think those changes are in the right direction, but it has to be precisely documented, as the auto-closing introduces more magic and an implicit understanding of the stuff. My main fright is that this can be pretty nasty for the existing users, as such bugs may happen only in specific usage (so be seen as non-deterministic). Maybe we could introduce some "v6-compatibility mode" that'd set it up as close as possible to previous behaviour? |
src/EventSourcingTests/append_events_with_optimistic_or_exclusive_locks.cs
Show resolved
Hide resolved
| public int Execute(NpgsqlCommand cmd) | ||
| { | ||
| Logger.OnBeforeExecute(cmd); | ||
| using var conn = _database.CreateConnection(); |
There was a problem hiding this comment.
SUGGESTION: Maybe we should put a circuit breaker for the default exception when the server is down. Doing reconnections may flood it and not allow it to start when multiple requests or instances will try to do retries.
There was a problem hiding this comment.
I was going to agree with you, but changed my mind after thinking about it more. I'd recommend folks put the circuit breaker higher up in the stack. If folks were using Wolverine for example, I'd say you put the circuit breaker on the message processing level and do that with Wolverine itself.
Maybe an example in the docs though so you can opt into that yourself w/ some copy/paste code.
|
"I assume that the only way to set a reusing connection now is to either:" -- not quite. If you call "I'd love to get rid of those injections with the Connection and lazy-loaded stuff, but I'm not sure if that won't be too big a change for some of the users and would make them stay on the older versions. On the other hand, that's more of a guess for now on how many people are using such a way."
"We still need to do benchmarks if there's no performance degradation, as the opening connection can be quite costly, especially around the non-pooled usage." Of course. I was working last night on battery power (and a cold). I'll run the benchmarks today and report back. I'm not buying that there's going to be any "performance degradation". The connection pooling is all underneath Npgsql itself, and I don't think this change will have any significant impact on performance due to connection pooling. Besides, even under the old model we didn't open the connection until we had to. Under heavy load, I'd expect it to be the opposite as Npgsql won't have to use so many connections. And also, look much closer at the change in behavior around Polly w/ the static lambdas vs how we were using IRetryPolicy before. What we were doing with IRetryPolicy was pretty gross in terms of object allocations. I would expect the new model to be faster. "I think those changes are in the right direction, but it has to be precisely documented, as the auto-closing introduces more magic and an implicit understanding of the stuff. My main fright is that this can be pretty nasty for the existing users, as such bugs may happen only in specific usage (so be seen as non-deterministic). Maybe we could introduce some "v6-compatibility mode" that'd set it up as close as possible to previous behaviour?" I'd thought about the v6-compatible switch. That's relatively easy to do, but I'd do that w/ a follow up PR. I think that's a good idea. The old connection lifetime mechanics are essentially still there, it's a state pattern switch here |
|
@oskardudycz Another problem for a day soon I guess, the So we're still dead in the water w/ Project Aspire and using an underlying NpgsqlDataSource. I think maybe we could beat that after the IConnectionLifetime changes in this PR, but previously, that prevents Marten from releasing connections somehow and kaboom. |
…it down to 5 seconds
…ers enabled. Closes GH-2912
