intelligenthack / intelligentcache

A distributed cache backed by Redis
MIT License
18 stars 2 forks source link

Support default duration #22

Closed gabrieldevoto closed 3 years ago

gabrieldevoto commented 3 years ago

The change aims to support default cache duration globally by setting up with CacheDuration property. Now duration doesn't need to be pass every time GetSet method is called.

Also changes where made to check that calculateValue method doesn't return null values. A NullRefereceException with a proper message is thrown in case it happens.

Readme file was updated with this new changes.

aaubry commented 3 years ago

Does it make sense to support a default duration ? Previously when no duration was passed, an infinite duration was assumed. @sklivvz What are your thoughts about this ?

tejuguasu commented 3 years ago

Note that DateTimeOffset.UtcNow.Add(TimeSpan.MaxValue) throws an exception because DateTime overflows (MaxValue is ≈25,000 years)

sklivvz commented 3 years ago

@aaubry I personally think that duration should be compulsory:

aaubry commented 3 years ago

@aaubry I personally think that duration should be compulsory:

* a default duration seems inappropriate on the basis that duration is never cache-specific, but it's item-specific (staleness is a property of an object in cache not of an entire cache)

* an infinite duration seems hardly an appropriate default (note: this does not mean it should not exist, just that it should not be the default)

Then it follows that we shouldn't make this change in the first place, correct ?

aaubry commented 3 years ago

Note that DateTimeOffset.UtcNow.Add(TimeSpan.MaxValue) throws an exception because DateTime overflows (MaxValue is ≈25,000 years)

Thanks. Since I am already writing tests for MemoryCache, I'll add a test for this case and fix it separately.

sklivvz commented 3 years ago

Then it follows that we shouldn't make this change in the first place, correct ?

What are the arguments in favor of having a default (e.g. in a property) or a fixed infinite default? I've looked at the slack discussions but the ideas were presented without an actual motivation. What is the problem we are trying to solve?

tejuguasu commented 3 years ago

IMO, reduce by 1 the passed parameter count since the current usage would use the same value every time.

gabrieldevoto commented 3 years ago

The problem would be: if I want a default duration for my hole application I can't set it globally, I have to pass it EVERY time I call the GetSet Method.

sklivvz commented 3 years ago

The idea is that cached items have a certain specific time after which they become stale. If you cache the weather, it needs to be updated every hour because the source changes every hour. If you cache a very commonly hit content out of a large set of possible choices, the cache time determines how fast the system can respond to changes in request patterns... and so on.

gabrieldevoto commented 3 years ago

That is right. But, for example, in our site we are saving site content that is changed once in a blue moon, so I thought it was easier to set it once.

Yet, if you think is ok to pass it every time we can live it like that. It was just an improvement I thought would be useful.

aaubry commented 3 years ago

Closing this as we decided to not have efault durations.