Shopify
Move delete failure detection to the cache fetcher.
@daniellaniyo, @Sirupsen please review
Fixes #211
Problem
IdentityCache::MemoizedCacheProxy#delete was assuming the delete failed if cache_fetcher.delete returned falsey. However, that falsey value could either mean the memcached request failed or the key wasn't present, and different cache stores aren't consistent about whether nil or false means as a return value for their delete method as I detailed in https://github.com/Shopify/identity_cache/issues/211#issuecomment-137626605.
This was confusing because we were logging something like [IdentityCache] delete failed for IDC:5:blob:App:18240036282024875059:41 which sounds like a probably with expiring the cache, but it could just mean that the value wasn't cached yet.
Solution
If IdentityCache::MemoizedCacheProxy#cache_fetcher is an IdentityCache::CacheFetcher, then it writes a IdentityCache::DELETED value instead of using a delete memcached request. In that case the return value isn't as ambiguous, it just returns falsey if the request failed. So, I was able to move the failure detection into IdentityCache::CacheFetcher#delete where we can rely on it and put a generic [IdentityCache] delete for #{key} message in IdentityCache::FallbackFetcher#delete.
Sure, I'll add a test for this. Also, I think we should probably use a higher log level for logging that the delete failed, since normally we wouldn't log at the debug level in production, yet this could be useful to investigating cache corruption.
So, TL;DR the one reason to do this is to make the non-CAS capable adaptors logging less noisy? Or is most of that logging just useless now?
This LGTM in isolation but I wonder if we should make CAS a requirement it has improved a bunch of cache desynchronization things for us
cc @fbogsany
