Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Caching api review changes #66

Merged
merged 1 commit into from
May 21, 2015
Merged

Caching api review changes #66

merged 1 commit into from
May 21, 2015

Conversation

kichalla
Copy link
Member

@Tratcher

Please note:

  • The changes to Redis cache implementation is incomplete...wanted to get initial input on the MemoryCache changes.

if (AbsoluteExpirationRelativeToNow != null)
{
throw new InvalidOperationException(
$"Cannot set both the properties '{nameof(AbsoluteExpirationRelativeToNow)}' " +
Copy link
Member Author

Choose a reason for hiding this comment

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

Should get from resources?

@Tratcher
Copy link
Member

⌚ So far so good, but I only reviewed the memory cache components. Distributed Cache and Redis haven't completed API review yet, so you might as well leave them out of the PR until we get memory cache finished.

Let me know when you're ready to work on entry links.

return new object();
});
result = cache.Set(
key,
Copy link
Member

Choose a reason for hiding this comment

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

Can you have an example of a set call that doesn't chain the options into a single argument?

var options  = new CacheEntryOptions()
                                .RegisterPostEvictionCallback((echoKey, value, reason, substate) => 
                                     Console.WriteLine(echoKey + ": '" + value + "' was evicted due to " + reason), state: null);

result = cache.Set(key, new object(), options);

But with better formatting

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure

@kichalla kichalla changed the title Caching api review changes - Part 1 Caching api review changes May 14, 2015

Task<bool> TryGetValueAsync(string key, out byte[] value);

void Set(string key, byte[] value, CacheEntryOptions options);
Copy link
Member

Choose a reason for hiding this comment

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

Distributed cache will not use the same options type as memory cache, many of the options are different. Give it its own options type like before.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, this becomes DistributedCacheEntryOptions as per yesterday's review.

@kichalla kichalla force-pushed the kiran/caching-changes branch 3 times, most recently from ffdae36 to a583c86 Compare May 15, 2015 23:38
@kichalla
Copy link
Member Author

Updated. /cc: @Tratcher

@kichalla kichalla force-pushed the kiran/caching-changes branch 3 times, most recently from 59a8ef3 to bbe53db Compare May 18, 2015 19:26
@@ -30,5 +44,14 @@ public void SetAbsoluteExpiration(DateTimeOffset absoluteExpiration)
AbsoluteExpiration = absoluteExpiration;
}
Copy link
Member

Choose a reason for hiding this comment

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

IEntryLink.SetAbsoluteExpiration probably needs a new name like AddAbsoluteExpiration that indicates it's not strictly an overwrite.

@Tratcher
Copy link
Member

:shipit: pending the redis test run.

@kichalla kichalla merged commit d630272 into dev May 21, 2015
@kichalla kichalla deleted the kiran/caching-changes branch May 22, 2015 22:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants