Redis Connection across Tenants #13531
Conversation
src/OrchardCore.Modules/OrchardCore.Redis/Options/RedisCacheOptionsSetup.cs
Show resolved
Hide resolved
|
We've completed a performance test against this and the results were so shocking that I'm hesitant to trust them until we get this tested in other environments. Basically, we went from 1800 connections during the performance test down to 34 connections. As a reminder, we aren't testing the multiple connection string part of this as our tenants within an environment share the same Redis server instance. Our automation tests will be running against this tonight so we'll see what kind of results we see there tomorrow as these test out the editing side of things which isn't touched in our performance tests. |
|
Hope it will still work but looks promising ;) |
| /// <summary> | ||
| /// Host level factory allowing to share a <see cref="IDatabase"/> across tenants. | ||
| /// </summary> | ||
| public sealed class RedisDatabaseFactory : IRedisDatabaseFactory, IDisposable, IAsyncDisposable |
There was a problem hiding this comment.
This should not be in Redis.Abstractions. I understand why it's there, and the OC.Infra project should not depend on Redis.
| await _semaphore.WaitAsync(); | ||
| try | ||
| { | ||
| _database ??= (await ConnectionMultiplexer.ConnectAsync(options)).GetDatabase(); |
There was a problem hiding this comment.
I saw your comment about assuming all tenants use the same redis connection string. I wouldn't make this assumption and just create a dictionary here.
|
@sebastienros Okay I will do the changes this week end Do we want it for 1.6.0? Where do you want to define and register this host level Redis database factory? Or maybe redefine it in the module and then use a static dictionary? |
| return database; | ||
| } | ||
|
|
||
| await _semaphore.WaitAsync(); |
There was a problem hiding this comment.
It's a concurrent dictionary. Maybe use TryGetOrAdd(Lazy)
There was a problem hiding this comment.
Yes I thought about it but at some point you were not a fan of using Lazy ;)
Okay will do
There was a problem hiding this comment.
Depends on the context, here I think it's important to not create multiple connections for the same string, so I agree with the locking, and because we have a CD I think the Lazy pattern should be used.
Fixes #12822
So here we register a host level factory allowing all tenants to share the same Redis Connection.
For now I didn't implement a connection pool, we assume that all tenants use the same config to connect to Redis, the difference being still done by prefixing the keys according to the tenant names.
For info I needed to wrap the aspnetcore
RedisCacheso that it doesn't dispose the shared Redis Connection each time a tenant is reloaded or released (e.g. on enabling a feature).