3
\$\begingroup\$

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<Type, List<object>> _cache = new ConcurrentDictionary<Type, List<object>>(); /// <summary> /// Load {T} into object cache from Data Store. /// </summary> /// <typeparam name="T">class</typeparam> public static void LoadIntoCache<T>() where T : class { _cache[typeof(T)] = RedisGetAll<T>().Cast<object>().ToList(); } /// <summary> /// Add single {T} into cache and Data Store. /// </summary> /// <typeparam name="T">class</typeparam> /// <param name="entity">class object</param> public static void Create<T>(T entity) where T : class { List<object> list; if (!_cache.TryGetValue(typeof(T), out list)) { list = new List<object>(); } list.Add(entity); _cache[typeof(T)] = list; RedisStore<T>(entity); } /// <summary> /// Find Single {T} in object cache. /// </summary> /// <typeparam name="T">class</typeparam> /// <param name="predicate">linq statement</param> /// <returns></returns> public static T Read<T>(Func<T, bool> predicate) where T : class { List<object> list; if (_cache.TryGetValue(typeof(T), out list)) { return list.Cast<T>().Where(predicate).FirstOrDefault(); } return null; } /// <summary> /// Tries to update or Add entity to object cache and Data Store. /// </summary> /// <typeparam name="T">class</typeparam> /// <param name="predicate">linq expression</param> /// <param name="entity">entity</param> public static void Update<T>(Func<T, bool> predicate, T entity) where T : class { List<object> list; if (_cache.TryGetValue(typeof(T), out list)) { // Look for old entity. var e = list.Cast<T>().Where(predicate).FirstOrDefault(); if (e != null) { list.Remove(e); } // Regardless if object existed or not we add it to our Cache / Data Store. list.Add(entity); _cache[typeof(T)] = list; RedisStore<T>(entity); } } /// <summary> /// Delete single {T} from cache and Data Store. /// </summary> /// <typeparam name="T">class</typeparam> /// <param name="entity">class object</param> public static void Delete<T>(T entity) where T : class { List<object> list; if (_cache.TryGetValue(typeof(T), out list)) { list.Remove(entity); _cache[typeof(T)] = list; RedisDelete<T>(entity); } } /// <summary> /// Find List<T>(predicate) in cache. /// </summary> /// <typeparam name="T">class</typeparam> /// <param name="predicate">linq statement</param> /// <returns></returns> public static List<T> FindBy<T>(Func<T, bool> predicate) where T : class { List<object> list; if (_cache.TryGetValue(typeof(T), out list)) { return list.Cast<T>().Where(predicate).ToList(); } return new List<T>(); } /// <summary> /// Find All {T} /// </summary> /// <typeparam name="T"></typeparam> /// <returns>List<T></returns> public static List<T> All<T>() where T : class { return RedisGetAll<T>().ToList(); } #region Redis Commands public static long Next<T>() where T : class { using (var ctx = m.GetClient()) return ctx.As<T>().GetNextSequence(); } private static void RedisDelete<T>(T entity) where T : class { using (var ctx = m.GetClient()) ctx.As<T>().Delete(entity); } private static T RedisFind<T>(long id) where T : class { using (var ctx = m.GetClient()) return ctx.As<T>().GetById(id); } private static IList<T> RedisGetAll<T>() where T : class { using (var ctx = m.GetClient()) return ctx.As<T>().GetAll(); } private static void RedisStore<T>(T entity) where T : class { using (var ctx = m.GetClient()) ctx.Store<T>(entity); } #endregion } /// <summary> /// Simple Test Entity /// </summary> public class User { public User() { Id = Repository.Next<User>(); } public long Id { get; private set; } public string Name { get; set; } } } 

Sample Test Application:

using System; using System.Collections.Generic; using System.Linq; using System.Linq.Expressions; using System.Text; using System.Collections.Concurrent; using Datamodel; namespace Test { class Program { static void Main(string[] args) { // Spool Redis entities into cache. Repository.LoadIntoCache<User>(); //// We do not touch sequence, by running example multiple times we can see that sequence will give Users new unique Id. //// Empty data store. Console.WriteLine("Our User Data store should be empty."); Console.WriteLine("Users In \"Database\" : {0}\n", Repository.All<User>().Count); //// Add imaginary users. Console.WriteLine("Adding 100 imaginairy users."); for (int i = 0; i < 99; i++) Repository.Create<User>(new User { Name = "Joachim Nordvik" }); //// We should have 100 users in data store. Console.WriteLine("Users In \"Database\" : {0}\n", Repository.All<User>().Count); // Lets print 10 users from data store. Console.WriteLine("Order by Id, Take (10) and print users."); foreach (var u in Repository.All<User>().OrderBy(z => z.Id).Take(10)) { Console.WriteLine("ID:{0}, Name: {1}", u.Id, u.Name); // Lets update an entity. u.Name = "My new Name"; Repository.Update<User>(x => x.Id == u.Id, u); } // Lets print 20 users from data store, we already edited 10 users. Console.WriteLine("\nOrder by Id, Take (20) and print users, we previously edited the users that we printed lets see if it worked."); foreach (var u in Repository.All<User>().OrderBy(z => z.Id).Take(20)) { Console.WriteLine("ID:{0}, Name: {1}", u.Id, u.Name); } // Clean up data store. Console.WriteLine("\nCleaning up Data Store.\n"); foreach (var u in Repository.All<User>()) Repository.Delete<User>(u); // Confirm that we no longer have any users. Console.WriteLine("Confirm that we no longer have User entities in Data Store."); Console.WriteLine("Users In \"Database\" : {0}\n\n", Repository.All<User>().Count); Console.WriteLine("Hit return to exit!"); Console.Read(); } } } 
\$\endgroup\$
0

    1 Answer 1

    4
    \$\begingroup\$

    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.

    1. 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.

    2. 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.

    3. 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).

    4. 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<object> 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<T>().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<T>(entity); } 
    5. 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.

    \$\endgroup\$
    2
    • \$\begingroup\$Thanks for your feedback! I have a rather huge data-model with very simple types, using EF I found that the performance was horrid much could probably be said or done about my EF implementation but from monitoring the SQL server it became very clear that I wanted to move the data into a cache as I was getting 100 000:1 read / write ratio. As I was saying I have a huge data-model with simple types. And it was becoming quite hard to maintain and migrate whenever I wanted to make changed to the model.\$\endgroup\$
      – Joachim
      CommentedJun 14, 2015 at 14:42
    • \$\begingroup\$1. Nothing else will communicate with Redis except the application. Idea is that I use Redis to keep the data model persistent each time I start the application. 2. Partly explained in 1 as its by design that I only want to check in changes during runtime. 3. Your correct, I want to use a single cache object as the local data-store. 4. I'm going to be honest I was not how to do this in a clean and more correct manner. 5. Again your absolutely correct, i wanted to update my first post saying that I will need to refactor the Redis portion of the code adding exception handling.\$\endgroup\$
      – Joachim
      CommentedJun 14, 2015 at 14:43

    Start asking to get answers

    Find the answer to your question by asking.

    Ask question

    Explore related questions

    See similar questions with these tags.