EntityList cache issues ...

[Bob Morley]

Today I spent some time figuring out why our entity list cache was not working as expected.  We had a PartyContent entity that was fetched in a entity-and with use-cache="true".  However, when the PartyContent was updated the cache was not properly flushed in the EntityList cache.  Here is what I found:

1) Cache.remove(GenericEntity entity) is being called with the updated GenericEntity.  This is used to call entityListCache.storeHook(entity, null) and the signature expects to have oldvalue, newvalue.  Ultimately it iterates through the conditions and attempts to programatically apply them to the "old value".  If there is match that condition is removed from the cache.  The trouble of course, is that it is using the new value which may cause the condition to no longer match (as in our case).

2) During the comparison logic for the condition (above) the system ultimately gets down to EntityJoinOperator.entityMatches(GenericEntity, lhs, rhs).  This object has a "shortCircuitValue" (basically when should I stop checking for this condition).  For our AND the short circuit value would be "false" as in if I hit false on the left-hand-side there is no point in checking the right-hand-side, lets leave now with FALSE.  The trouble was the code looked like:

if (lhs.entityMatches(entity)) return shortCircuitValue;

instead of

if (lhs.entityMatches(entity) == shortCircuitValue) return shortCircuitValue;

This pattern is in a few places, it was just wrong in this method and mapMatches method (right below).

SO, I have a clear fix for #2 but my solution for #1 isn't great (I go fetch the entity from the database so I can get the "old value").  I would like to talk with someone with lots of caching experience here to make sure I am not off base.  Also, wonder if there is a way to get the original version of the GenericEntity without doing a re-fetch.  I was considering an solution where the fields would be changed to a MapStack where the updated fields would be on the top of the stack (always a stack of 2).

Thoughts?

[David E. Jones-2]

On Aug 27, 2009, at 2:45 PM, Bob Morley wrote:

>
> Today I spent some time figuring out why our entity list cache was not
> working as expected.  We had a PartyContent entity that was fetched  
> in a
> entity-and with use-cache="true".  However, when the PartyContent was
> updated the cache was not properly flushed in the EntityList cache.  
> Here is
> what I found:
>
> 1) Cache.remove(GenericEntity entity) is being called with the updated
> GenericEntity.  This is used to call  
> entityListCache.storeHook(entity, null)
> and the signature expects to have oldvalue, newvalue.  Ultimately it
> iterates through the conditions and attempts to programatically  
> apply them
> to the "old value".  If there is match that condition is removed  
> from the
> cache.  The trouble of course, is that it is using the new value  
> which may
> cause the condition to no longer match (as in our case).
>
> 2) During the comparison logic for the condition (above) the system
> ultimately gets down to  
> EntityJoinOperator.entityMatches(GenericEntity, lhs,
> rhs).  This object has a "shortCircuitValue" (basically when should  
> I stop
> checking for this condition).  For our AND the short circuit value  
> would be
> "false" as in if I hit false on the left-hand-side there is no point  
> in
> checking the right-hand-side, lets leave now with FALSE.  The  
> trouble was
> the code looked like:
>
> if (lhs.entityMatches(entity)) return shortCircuitValue;
>
> instead of
>
> if (lhs.entityMatches(entity) == shortCircuitValue) return
> shortCircuitValue;
>
> This pattern is in a few places, it was just wrong in this method and
> mapMatches method (right below).
>
> SO, I have a clear fix for #2 but my solution for #1 isn't great (I  
> go fetch
> the entity from the database so I can get the "old value").  I would  
> like to
> talk with someone with lots of caching experience here to make sure  
> I am not
> off base.  Also, wonder if there is a way to get the original  
> version of the
> GenericEntity without doing a re-fetch.  I was considering an  
> solution where
> the fields would be changed to a MapStack where the updated fields  
> would be
> on the top of the stack (always a stack of 2).
>
> Thoughts?

The GenericEntity object keeps a Map with original values if it was  
loaded from the database and then changed, so you should be able to  
use that (it does seem weird that it isn't used already!).

-David

[Bob Morley]

I will create a JIRA for this issue and then write a unit test that exposes the issue along with a patch that fixes it up.  The fixes are already in place for us, so it will just be a matter of the unit test.  For interest sake, here are the few lines that show the issue:

DelegatorImpl -> store(GenericValue value, ...) (line 2527)
        this.clearCacheLine(value);

Cache -> remove(GenericEntity entity) (line 114+)
        GenericValue oldEntity = entityCache.remove(entity.getPrimaryKey());
        entityListCache.storeHook(entity, null);
        entityObjectCache.storeHook(entity, null);

AbstractEntityConditionCache -> storeHook(..., List<T1> oldValues, ...) (line 197+)
                Iterator<T1> oldValueIter = oldValues.iterator();
                while (oldValueIter.hasNext() && !shouldRemove) {
                    T1 oldValue = oldValueIter.next();
                    if (condition.mapMatches(getDelegator(), oldValue)) {

A jumped a few of the methods in the callstack, but suffice to say the delegator passes in the modified entity, the cache takes it and provides it to storeHook, and the storeHook parameter expects it is the oldValue.  The iteration is doing a condition.mapMatches against these (actually) modified values and as a result, does not find a match ... so the condition is ok ... so it is not flushed from the cache.

You may be wondering if we can just have Cache -> remove pass in the oldEntity from the entityCache.remove call.  The trouble is that in a lot of cases the entity list that is retrieved contains a pile of entities that (themselves) may have never been independantly cached.  My "hack" was to check for null here and then do a delegator.findByPrimaryKey to grab the real old one and go with that.  Naturally we do not want the overhead of re-retrieving the entity again - hence the original comment.  :)

I will cut the JIRA now and should be able to work out the solution either tomorrow or on Monday (EST).

Thanks!
Bob

David E Jones-4 wrote

The GenericEntity object keeps a Map with original values if it was  
loaded from the database and then changed, so you should be able to  
use that (it does seem weird that it isn't used already!).

-David

[Bob Morley]

I have attached to files to the patch.  The one has a unittest that exposes the problem with the caching.  The second contains two fixes that resolve the issue (and cause the unittest to pass).