Make DistributedCacheManager more robust

[dzmitryk-kontent-ai]

Motivation

Which issue does this fix? Fixes #352

If no issue exists, what is the fix or new feature? Were there any reasons to fix/implement things that are not obvious?

Checklist

• [ ] Code follows coding conventions held in this repo
• [ ] Automated tests have been added
• [ ] Tests are passing
• [ ] Docs have been updated (if applicable)
• [ ] Temporary settings (e.g. variables used during development and testing) have been reverted to defaults

How to test

If manual testing is required, what are the steps?

[codecov-commenter]

Codecov Report

Merging #358 (8e71304) into master (79a71bd) will increase coverage by 0.02%. The diff coverage is 95.45%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master     #358      +/-   ##
==========================================
+ Coverage   88.87%   88.90%   +0.02%     
==========================================
  Files         125      125              
  Lines        2679     2695      +16     
  Branches      341      343       +2     
==========================================
+ Hits         2381     2396      +15     
  Misses        169      169              
- Partials      129      130       +1     

Impacted Files	Coverage Δ
...ent.Ai.Delivery.Caching/DistributedCacheManager.cs	66.66% <92.85%> (+5.30%)	:arrow_up:
...ontent.Ai.Delivery.Caching/DeliveryCacheOptions.cs	100.00% <100.00%> (ø)
...ching/Extensions/DeliveryCacheOptionsExtensions.cs	100.00% <100.00%> (ø)
....Delivery.Caching/Factories/CacheManagerFactory.cs	100.00% <100.00%> (ø)
...iveryClient/DeliveryClientBuilderImplementation.cs	100.00% <100.00%> (ø)
Kontent.Ai.Delivery/DeliveryClient.cs	82.55% <100.00%> (+0.14%)	:arrow_up:
...Delivery/Extensions/ServiceCollectionExtensions.cs	91.22% <100.00%> (+0.15%)	:arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[Simply007]

Do we even need to make this resilience configurable?

[Simply007]

Another alternative to cut out ILoogerFactory logic from the PR and rely on user to register ILoogerFactory by himself and cut the option for resilience out and set default behavior to fallback with log.

• User knows about fail (because of logs) -> will be documented and do not get the exception.

Or add WithLogger/WithLoggerFactory similarly we have with WithRetryPolicyProvider, WithPropertyManager .

[gormal]

Do we even need to make this resilience configurable?

When a customer's cache stops functioning, it is their decision whether to continue using DAPI (and incurring costs) or to crash the application and resolve the issue quickly.

• Tests - I might shrink the BrokenDistributedCache.cs implementation to the test to a bare minimum (no check if statements and all async methods directly calling Task.FromResult(NonAsyncMethod())) - @gormal - do you agree?

Yeah, sounds great.

• Extend the description od CacheTypeEnum.Distributed with the explanation that is cache in unavailable - it will fail
• Add something like CacheTypeEnum.DistributedWIthApiFallback (I am open to a better name) which would start fallback to the API if Cache is not available.

I would strongly advise against unifying these settings. It's easy to unify but very hard to separate in the future. Maybe there are some other options how to achieve the independence of these settings.

Another alternative to cut out ILoogerFactory logic from the PR and rely on user to register ILoogerFactory by himself and cut the option for resilience out and set default behavior to fallback with log.

• User knows about fail (because of logs) -> will be documented and do not get the exception.

Or add WithLogger/WithLoggerFactory similarly we have with WithRetryPolicyProvider, WithPropertyManager .

I like the WithLoggerFactory idea.

[Simply007]

• let's keep BrokenDistributedCache as is
• keep the setting as it is for CacheOptions - lat' take a look at this on review, if something pop up
• introduce WithLoggerFactory -> I am open to keeping it as a separate issue (but remove the current logic with login in this PR)

[dzmitryk-kontent-ai]

@Simply007 , please go through the code, especially documentation changes. Since previous review I added WithLoggerFactory extension method which allows to register ILoggerFactory implementation during DeliveryClient creation