Support Distributed Caching bypass capability #10297

ShaneCourtrille · 2021-09-17T17:55:16Z

As per the request in #10197 I've created an initial PR so we can discuss the changes necessary to improve system stability when Redis is unavailable.

I'm still unsure on the delay when an exception occurs. If it should move into config or if it should use an increasing time as per the RedisLock.

Encapsulates distributed caching and adds cache bypass when Redis has an issue

dnfadmin · 2021-09-17T17:55:29Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ ShaneCourtrille sign now
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

Skrypt · 2021-09-18T16:22:36Z

I like the idea of making it redundant to make sure it will work properly. Though, I think the implementation you did forces us to use a Redis Cache while the IDistributedCache should be abstract.

Skrypt · 2021-09-18T16:24:42Z

src/OrchardCore/OrchardCore.Abstractions/Caching/Distributed/IDistributedCache.cs

+
+namespace OrchardCore.Caching.Distributed
+{
+    public interface IDistributedCache


This is kind of the same as
https://github.com/aspnet/Caching/blob/master/src/Microsoft.Extensions.Caching.Abstractions/IDistributedCache.cs

Yes, the problem is that if you use the same interface and AddStackExchangeRedis() you either have to extract/wrap the dependency it creates OR stop using that call and recreate it.

I actually thought of a solution last night.. in the constructor for DocumentManager it could ask for a MS IDistributedCache and then wrap it as long as I changed the RedisDistributedCache into a more generic version.

jtkech · 2021-09-19T04:18:15Z

src/OrchardCore.Modules/OrchardCore.Redis/Caching/RedisDistributedCache.cs

+using Microsoft.Extensions.Caching.Distributed;
+using OrchardCore.Modules;
+
+namespace OrchardCore.Redis.Caching


I would move it to the Services folder where we already have RedisBus and RedisLock.

jtkech · 2021-09-19T05:01:25Z

src/OrchardCore.Modules/OrchardCore.Redis/Caching/RedisDistributedCache.cs

+        public RedisDistributedCache(Microsoft.Extensions.Caching.Distributed.IDistributedCache cache, IClock clock)
+        {
+            _underlyingCache = cache;
+            _clock = clock;


As stated by @Skrypt we don't need to define a new interface

Here we could inject IOptions<RedisCacheOptions> optionsAccessor so that we can create an inner _redisCache without having to resolve it, just by doing _redisCache = new RedisCache(optionsAccessor); https://github.com/dotnet/aspnetcore/blob/db4dc17779dbfbe96b94ad33bc02a368b4a94463/src/Caching/StackExchangeRedis/src/RedisCache.cs#L49

Then, in the redis module startup, not call AddStackExchangeRedisCache() that registers the RedisCache
https://github.com/dotnet/aspnetcore/blob/db4dc17779dbfbe96b94ad33bc02a368b4a94463/src/Caching/StackExchangeRedis/src/StackExchangeRedisCacheServiceCollectionExtensions.cs#L23-L40

And do what AddStackExchangeRedisCache() does unless that we register our wrapper

services.AddOptions(); // <= not necessary here services.Configure(setupAction); // <= idem, so I think only the following line services.Add(ServiceDescriptor.Singleton<IDistributedCache, RedisCacheService>());

Then we would have to implement IDisposable so that we can dispose our inner _redisCache.

public class RedisCacheService : IDistributedCache, IDisposable

Not sure yet of the best name ;)

Since the code doesn't actually depend on anything Redis specific what if I made it more generic?

The one problem I see with that is I don't have a 'clean room' code implementation of wrapping an existing dependency in services collection (I have an implementation I grabbed from somewhere else so the rights get tricky I think).

A semi dirty way around that would be to just wrap it in the DocumentManagers constructor in the if (distributedCache == typeof(MemoryCacheDistributedCache) {} block. I'm not the biggest fan of that though if someone can provide a clean services.AddDecorator(..) extension method then that would be preferred?

jtkech · 2021-09-19T05:09:20Z

src/OrchardCore.Modules/OrchardCore.Redis/Caching/RedisDistributedCache.cs

+            CheckDistributedErrorRetry();
+
+            if(_redisUnavailable)
+                return Array.Empty<byte>();


We have an .editorconfig file at the root so that e.g. under Visual studio you get some warnings about code style rules. Here, always use braces { and } for if blocks and on new lines

The .editorconfig file isn't working locally for me which is odd. It has the root=true and I've verified that my 'Follow project coding conventions' option is enabled.

jtkech · 2021-09-19T05:09:39Z

src/OrchardCore.Modules/OrchardCore.Redis/Caching/RedisDistributedCache.cs

+            if(_redisUnavailable)
+                return Array.Empty<byte>();
+
+            try {


open brace on new line

jtkech · 2021-09-19T05:10:21Z

src/OrchardCore.Modules/OrchardCore.Redis/Caching/RedisDistributedCache.cs

+            catch (Exception)
+            {
+                RedisCacheFailed();
+                return Array.Empty<byte>();


Maybe return null

jtkech · 2021-09-19T05:10:43Z

src/OrchardCore.Modules/OrchardCore.Redis/Caching/RedisDistributedCache.cs

+            CheckDistributedErrorRetry();
+
+            if(_redisUnavailable)
+                return Task.FromResult(Array.Empty<byte>());


Use braces and on newlines

jtkech · 2021-09-19T05:11:03Z

src/OrchardCore.Modules/OrchardCore.Redis/Caching/RedisDistributedCache.cs

+            if(_redisUnavailable)
+                return Task.FromResult(Array.Empty<byte>());
+
+            try {


open brace on new line

jtkech · 2021-09-19T05:11:18Z

src/OrchardCore.Modules/OrchardCore.Redis/Caching/RedisDistributedCache.cs

+
+            try {
+                return _underlyingCache.GetAsync(key, token);
+            } catch (Exception) {


Idem for all other ones

jtkech · 2021-09-19T05:46:08Z

@ShaneCourtrille

Just did a first review, will continue when I will have time

jtkech · 2021-09-19T05:50:27Z

src/OrchardCore.Modules/OrchardCore.Redis/Caching/RedisDistributedCache.cs

+                    return;
+
+                // TODO: move to configuration or should this be an increasing number like RedisLock?
+                _retryAt = _clock.UtcNow.AddSeconds(15);


Hmm, I will think about it

ShaneCourtrille · 2021-09-19T18:28:50Z

I like the idea of making it redundant to make sure it will work properly. Though, I think the implementation you did forces us to use a Redis Cache while the IDistributedCache should be abstract.

I don't think it forces Redis BUT there isn't actually anything Redis specific in the code now so the naming/location might not make sense anymore. Originally I was thinking I'd want to look for RedisConnectionExceptions and RedisConnectionTImeouts but since that never ended up happening I could just rename/move things so that its a more generic DistributedCacheErrorWrapper or whatever better name someone can come up with?

ShaneCourtrille · 2021-09-20T21:30:25Z

@jtkech @Skrypt I've created an alternative implementation in PR #10313 which is much cleaner except for one problem with dependency injection that I get around via a bit of ugly manual object creation.

Can you let me know which one makes more sense to focus on?

Piedone · 2024-01-18T21:58:22Z

Closing as per #10313 (comment).

Support Distributed Caching bypass capability

18569f6

Encapsulates distributed caching and adds cache bypass when Redis has an issue

ShaneCourtrille requested a review from jtkech as a code owner September 17, 2021 17:55

ShaneCourtrille mentioned this pull request Sep 17, 2021

DocumentManager.GetInternalAsync cannot handle Redis server maintenance #10197

Closed

Skrypt reviewed Sep 18, 2021

View reviewed changes

jtkech reviewed Sep 19, 2021

View reviewed changes

ShaneCourtrille mentioned this pull request Sep 20, 2021

Generic handler for distributed cache exceptions #10313

Closed

jtkech mentioned this pull request Sep 26, 2021

Cache Failover and Shared options #10338

Merged

Merge branch 'OrchardCMS:main' into main

ddf6de9

Piedone closed this Jan 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Distributed Caching bypass capability #10297

Support Distributed Caching bypass capability #10297

ShaneCourtrille commented Sep 17, 2021 •

edited

Loading

dnfadmin commented Sep 17, 2021

Skrypt commented Sep 18, 2021

Skrypt Sep 18, 2021

ShaneCourtrille Sep 19, 2021

jtkech Sep 19, 2021

jtkech Sep 19, 2021

ShaneCourtrille Sep 19, 2021

jtkech Sep 19, 2021

ShaneCourtrille Sep 20, 2021

jtkech Sep 19, 2021

jtkech Sep 19, 2021

jtkech Sep 19, 2021

jtkech Sep 19, 2021

jtkech Sep 19, 2021

jtkech Sep 19, 2021

jtkech commented Sep 19, 2021

jtkech Sep 19, 2021

ShaneCourtrille commented Sep 19, 2021

ShaneCourtrille commented Sep 20, 2021 •

edited

Loading

Piedone commented Jan 18, 2024

Support Distributed Caching bypass capability #10297

Support Distributed Caching bypass capability #10297

Conversation

ShaneCourtrille commented Sep 17, 2021 • edited Loading

dnfadmin commented Sep 17, 2021

Skrypt commented Sep 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtkech commented Sep 19, 2021

Choose a reason for hiding this comment

ShaneCourtrille commented Sep 19, 2021

ShaneCourtrille commented Sep 20, 2021 • edited Loading

Piedone commented Jan 18, 2024

ShaneCourtrille commented Sep 17, 2021 •

edited

Loading

ShaneCourtrille commented Sep 20, 2021 •

edited

Loading