Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add configuration by code for System.Runtime.Caching memory options #177 #228

Merged
merged 3 commits into from
Sep 25, 2019

Conversation

nflash
Copy link
Contributor

@nflash nflash commented May 2, 2018

In response to #177 the RuntimeMemoryCacheOptions class was created to allow configuring the System.Runtime.Caching.MemoryCache.

All overloads of the extension method WithSystemRuntimeCacheHandle with a RuntimeMemoryCacheOptions parameter also requires the instanceName parameter because as I understand it, it is not possible to change the Default cache settings.

But that's only one way to configure it:

  • Not sure if we have to add it to the old web/app configuration system as you can already configure it there anyways.
  • Configuration via MS.Extensions.Configuration, like json files. I can figure that one out later though.

Configuration with web/app configuration system still works as before and I think is the only way to configure the default cache.

For the MS.Extensions.Configuration I think I would need some guidance.

Copy link
Owner

@MichaCo MichaCo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good overall! ;)

For unit testing, can you add/change one of the shared cache configurations used by many other tests to use the new way of configuring this cache handle?

For example, change this one https://github.com/MichaCo/CacheManager/blob/dev/test/CacheManager.Tests/TestCacheManagers.cs#L332

Many other tests use that collection of different caches to run tests against it. That's basically a test matrix for many different configurations.

@nflash
Copy link
Contributor Author

nflash commented Jun 15, 2018

I had made the change to throw an exception when trying to set options for the Default cache. I have also added some more tests, including a test expecting the exception.

For unit testing, can you add/change one of the shared cache configurations used by many other tests to use the new way of configuring this cache handle?
For example, change this one https://github.com/MichaCo/CacheManager/blob/dev/test/CacheManager.Tests/TestCacheManagers.cs#L332

What kind of change do you suggest? Something like this?

public static ICacheManager<object> WithMemoryAndDictionaryHandles
            => CacheFactory.FromConfiguration<object>(
                BaseConfiguration
                    .Builder
                        .WithSystemRuntimeCacheHandle()
                            .EnableStatistics()
                        .And.WithSystemRuntimeCacheHandle("LimitedCacheHandle", new SystemRuntimeCaching.RuntimeMemoryCacheOptions() { PhysicalMemoryLimitPercentage = 20, CacheMemoryLimitMegabytes = 200 })
                            .EnableStatistics()
                            .WithExpiration(ExpirationMode.Absolute, TimeSpan.FromSeconds(1000))
                        .And.WithDictionaryHandle()
                            .EnableStatistics()
                        .And.WithDictionaryHandle()
                            .EnableStatistics()
                            .WithExpiration(ExpirationMode.Absolute, TimeSpan.FromSeconds(1000))
                    .Build());

I'm asking because I don't want to break any test by, for instance, setting a cache too small or by changing the number of cache handles

@MichaCo
Copy link
Owner

MichaCo commented Jun 15, 2018

Great!
Re. unit test. Yeah, that shouldn't break anything, that change should be good enough

@MichaCo MichaCo merged commit 800bebe into MichaCo:dev Sep 25, 2019
@ndrwrbgs ndrwrbgs mentioned this pull request Jan 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants