On Mon, 2010-07-12 at 15:04 -0400, Moore, Jonathan wrote: > Hi Oleg, I'm not sure what you're trying to accomplish here. >
This change was not driven by any practical requirement. I was just going through the code and the existing behavior of the #updateCacheEntry method took me completely by surprise. I am just trying to think of a better way of expressing the same contract. > You're right that sometimes multiple entries need to get updated at the > same time in a consistent fashion, which is why the callback is provided > (and then the BasicHttpCache marks this as a synchronized method). It > seems like your patch just adds a layer of explicit locking around what > already exists. > > It's also possible that the underlying cache mechanism does not support > the level of atomicity required to support an explicit getLock() (for > example, a memcached does not have a pessimistic lock, but it does have > an atomic compare-and-swap operation, allowing for optimistic locking). > The memcached implementation of #update would have to apply the > compare-and-swap with a possible retry on contention. > > At the same time, I think you are raising a valid concern here, which is > that the *interface* of HttpCacheUpdateCallback suggests that this is a > one-for-one swap of a single cache entry, but in reality sometimes other > things occur. The primary need for updating multiple records comes in > the case of responses with the Vary header, where we are maintaining > multiple variants as well as a "parent" entry that tracks them all (so > that we can invalidate all variants of a resource when necessary). > > It's also probably worth noting, though, that we don't actually *update* > more than one record at a time. The one place you changed in your patch > was the case where we got a *new* variant (i.e. a complete response), > which is just #put into the cache, completely overwriting anything > previously there, and then we *update* the parent to track the new > variantURI. > Right. But I am fairly sure in some cases one can still end up with two completely new entries added to the cache as a result of the method call. I even added a test case for that. I just thought that was confusing / non-obvious. > Right now non-variants, variants, and parent entries are all stored in > the same cache, and I think this is actually the crux of the problem; > there's probably a better domain model for this than the one we've got, > and I suspect that would make this problem go away. > > However, all things being equal, what we've got currently works, and I > would be hesitant to change the cache API without another actual cache > implementation to drive the API changes. > Let me try to take another cut at a slightly different API. I think something similar to Spring templates would provide a possibility of mutating multiple cache entries as a unit of work without exposing a lock and could potentially satisfy all parties. Cheers Oleg --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@hc.apache.org For additional commands, e-mail: dev-h...@hc.apache.org
