DoubleCache V2 #25
DoubleCache V2 #25
Conversation
|
@Ulriksen Do you have some description of what you want to get out of DoubleCache V2? Just so I can understand what's wrong with V1, what you want to make better, and if you're getting there. |
cbigsby
left a comment
cbigsby
left a comment
There was a problem hiding this comment.
Some of these comments are just ramblings of an idiot, but there are a few changes you should probably make, or at least think about.
| * PublishingCache - a decorator publishing cache changes. | ||
| * DoubleCache - a decorator wrapping a local and a remote cache. | ||
|
|
||
| \* using a custom proxy object holding the cache items. This is transparent to the client. |
There was a problem hiding this comment.
You might as well put this directly in the description for WrappingMemoryCache. It's not helpful to have it up here.
| { | ||
| public class HttpCache : ICacheAside | ||
| { | ||
| internal class CacheItemWrapper |
There was a problem hiding this comment.
You have this same internal class in a few different caches. Perhaps pull it out into a separate file so you can share it between classes?
| } | ||
| var wrapper = _cache.Get(key) as CacheItemWrapper; | ||
| if (wrapper != null) | ||
| return wrapper.Item as T; |
There was a problem hiding this comment.
There's an inconsistency introduced here now. Before, and in MemCache if you have a key "Users:123" that contains a User object, but you call Get<Product>("Users:123", () => ...); then it would see that as a cache miss and call your dataRetriever and overwrite the User object with the Product object. Now it will see a cache hit and return null.
The user is doing something incorrect, but do you want this to work the same as before or are you okay with it being different? I suppose in the RedisCache implementation it'll throw a SerializationException, so it's already not the same everywhere.
| * SubscribingCache - a decorator supporting push notifications of cache updates. | ||
| * ExistingItemSubscribingCache - a decorator supporting push notifications, which will only update cache if the item already exists. | ||
| * PublishingCache - a decorator publishing cache changes. | ||
| * DoubleCache - a decorator wrapping a local and a remote cache. |
There was a problem hiding this comment.
You might want to say what the default implementations DoubleCache uses when created from the factory.
|
|
||
| namespace DoubleCache | ||
| { | ||
| public class ExistingItemsSubscribingCache : ICacheAside |
There was a problem hiding this comment.
I wonder if eagerly filling the in-memory cache is that useful. What I have done for caching in the past is whenever an item changes (eg: I received a Service Bus message from another service) I just remove it from the cache. If-and-when something requests that item then we can fetch it again, but until it's actually needed that item is just taking up space.
| try | ||
| { | ||
| _redisCache.Add(key, await dataRetriever.Invoke(), staleTtl); | ||
| _redisCache.Add(key, await dataRetriever.Invoke().ConfigureAwait(false), staleTtl); |
There was a problem hiding this comment.
I don't think this will play well with PublishingCache. This will return the old value, so executed will be false, unless somehow dataRetriever.Invoke() returns before this method does, which is very unlikely.
I think the way you'd fix this is in the dataRetriever in PublishingCache you remove executed from the method and instead use Task.ContinueWith(_cachePublisher.NotifyUpdate(...)); to asynchronously notify the other instances.
| { | ||
| public class BinaryFormatterItemSerializer : IItemSerializer | ||
| { | ||
| private static ConcurrentDictionary<Type,MethodInfo> _typeCache = new ConcurrentDictionary<Type,MethodInfo>(); |
| <package id="MsgPack.Cli" version="0.6.5" targetFramework="net46" /> | ||
| <package id="StackExchange.Redis" version="1.0.488" targetFramework="net46" /> | ||
| <package id="MsgPack.Cli" version="0.8.1" targetFramework="net45" /> | ||
| <package id="StackExchange.Redis" version="1.2.3" targetFramework="net45" /> |
There was a problem hiding this comment.
Do you mean to downgrade the targetFramework to .NET 4.5?
| _cacheImplementation = new MemCache(TimeSpan.FromMinutes(1)); | ||
| } | ||
|
|
||
| public override void Cache_Null_Returns_Null() |
There was a problem hiding this comment.
The name of these tests do not describe what is actually happening.
No description provided.