Cached-object Store c# with Redis client for persistent storage

I have written a Cached-Object store with a Redis Client for persistent storage. The application that is going to use this is a heavy read application with the occasional write. I assume that entire model will contain somewhere around 500k - 1m entities. The tests I have done so far with a far more complicated model than I include as example shows that Entity Framework does not come close performance wise. Now that said I am curious if I have made any fatal mistakes in my Repository code.

External Framework ServiceStack.Redis

Repository:

```
using System;
using System.Collections.Generic;
using System.Collections.Concurrent;
using System.Linq;
using ServiceStack.Redis;

namespace Datamodel
{
public class Repository
{
private static readonly PooledRedisClientManager m = new PooledRedisClientManager();

readonly static IDictionary> _cache = new ConcurrentDictionary>();

///
/// Load {T} into object cache from Data Store.
///
/// class
public static void LoadIntoCache() where T : class
{
_cache[typeof(T)] = RedisGetAll().Cast().ToList();
}

///
/// Add single {T} into cache and Data Store.
///
/// class
/// class object
public static void Create(T entity) where T : class
{
List list;
if (!_cache.TryGetValue(typeof(T), out list))
{
list = new List();
}
list.Add(entity);
_cache[typeof(T)] = list;
RedisStore(entity);
}

///
/// Find Single {T} in object cache.
///
/// class
/// linq statement
///
public static T Read(Func predicate) where T : class
{
List list;
if (_cache.TryGetValue(typeof(T), out list))
{
return list.Cast().Where(predicate).FirstOrDefault();
}
return null

There are some causes for concern with this code. Could you elaborate a bit more on the intended use of this, as well as what you're hoping to gain from its use? That might help clear up some of the below points.

-
Performance

It's fast in your example, but many other cases may be slower than intended. Depends on your use case which is why some elaboration might help, but here it works nicely because you're only doing it on a small set. The LoadIntoCache method concerns me since it defeats the purpose of using Redis to some extent. You immediately lose all the faster querying ability and replace it with querying a List of all entities. What if you only need a subset, or need faster access of single object? Every read needs to traverse the whole retrieved List until it finds the entity. Also, a public LoadIntoCache seems unnecessary, and could be lazy-loaded when something was first requested.

-
Nothing invalidates the cache

What if data on the Redis instance changes? Your cache and it's state are incorrect and have no way of knowing it. You could be updating/deleting records that don't exist, inserting duplicates, etc. Also, calling LoadIntoCache() and accessing said cache may return different entities than a later ReadAll() call.

-
Everything is static

This class definitely has state, but everything is static. Is this because you want to make the cache a singleton? This makes it harder to test, harder to reuse, etc. (research why singletons are generally considered an anti-pattern).

-
It's not thread-safe

You're using a ConcurrentDictionary presumably for thread-safety, but your code doesn't really make use of the concurrent features. Consider your Update method code:

List list;

// Ok, this call is thread-safe:
if (_cache.TryGetValue(typeof(T), out list))
{
    // But this isn't. list is outside the thread-safety of your ConcurrentDictionary.
    var e = list.Cast().Where(predicate).FirstOrDefault();

    if (e != null)
    {
        list.Remove(e);
    }

    list.Add(entity);
    _cache[typeof(T)] = list;

    // Oops, something else deleted this. Or made other updates that you're stomping:
    RedisStore(entity);
}

-
No way for caller to know what happened

The caller to your Update method has no way of telling which of these things happened: 1) The cache wasn't storing objects of that type, so nothing happened. 2) You found an entity and replaced it. 3) You added a new entity. Making these methods single-use and have clear names would go a long way for clarity: Update( (throws exception if not found, and has an out argument for which entity was replaced), bool TryUpdateEntity( (returns bool if found and replaced), Add( (only adds). This applies for several other methods here.

StackExchange Code Review Q#93557, answer score: 4