Important: Entity list cache and GenericValue discussion

[Adrian Crum-3]

Last week I discovered a flaw in the EntityListCache implementation:
http://mail-archives.apache.org/mod_mbox/incubator-ofbiz-dev/201304.mbox/%3C516AC7B4.2020007@...%3E

To summarize: Entity conditions that are cached become stale when any
member of the cached list is changed - making the cache contents
invalid. In addition, GenericValues in the cached list are mutable -
which is inconsistent with the primary key cache, where GenericValues
from the cache are immutable.

I would like to fix this, but I think we should discuss it first.

One change would be to make the list member GenericValues immutable.
This will make the GenericValues retrieved from the entity list cache
consistent with the GenericValues retrieved from the primary key cache,
but it won't prevent an invalid cache (because the list member
GenericValue can be cloned and modified). Also, this change will likely
break a lot of code, because it is common to retrieve a list of
GenericValues from the cache and then make changes to the list members.
We could create a "transitional" GenericValue that would warn developers
when they modify a cached list member, and then switch to an immutable
GenericValue some time in the future.

To fix the stale cache problem, the cache instance can be made a
GenericValue listener in all of its list members - so any time a list
member is modified the cache will be cleared. This will keep the cache
valid, but there might be a performance hit. I'm open to other solutions
to this problem.

Any thoughts?

-Adrian

[Jacopo Cappellato-4]

It seems actually to be an issue rather than a feature (I can't think of a use case where this behavior would be useful); I have created a few test cases (similar to the one you have provided in the other thread) that further analyze your discovery but they don't add much to what you found (apart from confirming the risk of getting stale data).
However, when we start design/implement a refactoring of this part of the system, I would suggest that we also consider how to deal with similar scenarios in a clustered deployment (in fact many of the production deployment are based on clusters); the simplest use case could be: in a cluster, we have two OFBiz instances connected to the same database; in one instance the list is cached, in the other instance one of the generic values (that are part of the selection) is updated. A distributed cache system may help here.

Jacopo

On Apr 21, 2013, at 10:54 AM, Adrian Crum <[hidden email]> wrote:

> Last week I discovered a flaw in the EntityListCache implementation: http://mail-archives.apache.org/mod_mbox/incubator-ofbiz-dev/201304.mbox/%3C516AC7B4.2020007@...%3E
>
> To summarize: Entity conditions that are cached become stale when any member of the cached list is changed - making the cache contents invalid. In addition, GenericValues in the cached list are mutable - which is inconsistent with the primary key cache, where GenericValues from the cache are immutable.
>
> I would like to fix this, but I think we should discuss it first.
>
> One change would be to make the list member GenericValues immutable. This will make the GenericValues retrieved from the entity list cache consistent with the GenericValues retrieved from the primary key cache, but it won't prevent an invalid cache (because the list member GenericValue can be cloned and modified). Also, this change will likely break a lot of code, because it is common to retrieve a list of GenericValues from the cache and then make changes to the list members. We could create a "transitional" GenericValue that would warn developers when they modify a cached list member, and then switch to an immutable GenericValue some time in the future.
>
> To fix the stale cache problem, the cache instance can be made a GenericValue listener in all of its list members - so any time a list member is modified the cache will be cleared. This will keep the cache valid, but there might be a performance hit. I'm open to other solutions to this problem.
>
> Any thoughts?
>
> -Adrian
>
>

[Adrian Crum-3]

Thanks Jacopo. I haven't looked into the entity cache implementation
thoroughly, but I was under the impression that it can be configured to
be distributed.

I have the fix working on my local copy. As you can see, I have made
some related changes already and will be making some more, but I won't
commit the fix until next weekend - to give everyone a chance to respond.

-Adrian

On 4/22/2013 9:33 AM, Jacopo Cappellato wrote:

> It seems actually to be an issue rather than a feature (I can't think of a use case where this behavior would be useful); I have created a few test cases (similar to the one you have provided in the other thread) that further analyze your discovery but they don't add much to what you found (apart from confirming the risk of getting stale data).
> However, when we start design/implement a refactoring of this part of the system, I would suggest that we also consider how to deal with similar scenarios in a clustered deployment (in fact many of the production deployment are based on clusters); the simplest use case could be: in a cluster, we have two OFBiz instances connected to the same database; in one instance the list is cached, in the other instance one of the generic values (that are part of the selection) is updated. A distributed cache system may help here.
>
> Jacopo
>
> On Apr 21, 2013, at 10:54 AM, Adrian Crum <[hidden email]> wrote:
>
>> Last week I discovered a flaw in the EntityListCache implementation: http://mail-archives.apache.org/mod_mbox/incubator-ofbiz-dev/201304.mbox/%3C516AC7B4.2020007@...%3E
>>
>> To summarize: Entity conditions that are cached become stale when any member of the cached list is changed - making the cache contents invalid. In addition, GenericValues in the cached list are mutable - which is inconsistent with the primary key cache, where GenericValues from the cache are immutable.
>>
>> I would like to fix this, but I think we should discuss it first.
>>
>> One change would be to make the list member GenericValues immutable. This will make the GenericValues retrieved from the entity list cache consistent with the GenericValues retrieved from the primary key cache, but it won't prevent an invalid cache (because the list member GenericValue can be cloned and modified). Also, this change will likely break a lot of code, because it is common to retrieve a list of GenericValues from the cache and then make changes to the list members. We could create a "transitional" GenericValue that would warn developers when they modify a cached list member, and then switch to an immutable GenericValue some time in the future.
>>
>> To fix the stale cache problem, the cache instance can be made a GenericValue listener in all of its list members - so any time a list member is modified the cache will be cleared. This will keep the cache valid, but there might be a performance hit. I'm open to other solutions to this problem.
>>
>> Any thoughts?
>>
>> -Adrian
>>
>>

[Jacopo Cappellato-4]

On Apr 22, 2013, at 10:45 AM, Adrian Crum <[hidden email]> wrote:

> Thanks Jacopo. I haven't looked into the entity cache implementation thoroughly, but I was under the impression that it can be configured to be distributed.

I don't remember the details but the issue you have discovered for sure also happens in a clustered deployment; and the fix you are proposing would not address the clustered deployment (if I am not missing something).

Jacopo

[Jacques Le Roux]

From: "Adrian Crum" <[hidden email]>
> Thanks Jacopo. I haven't looked into the entity cache implementation
> thoroughly, but I was under the impression that it can be configured to
> be distributed.

Yes it is https://cwiki.apache.org/confluence/display/OFBIZ/Distributed+Entity+Cache+Clear+Mechanism

Jacques
 

> I have the fix working on my local copy. As you can see, I have made
> some related changes already and will be making some more, but I won't
> commit the fix until next weekend - to give everyone a chance to respond.
>
> -Adrian
>
> On 4/22/2013 9:33 AM, Jacopo Cappellato wrote:
>> It seems actually to be an issue rather than a feature (I can't think of a use case where this behavior would be useful); I have created a few test cases (similar to the one you have provided in the other thread) that further analyze your discovery but they don't add much to what you found (apart from confirming the risk of getting stale data).
>> However, when we start design/implement a refactoring of this part of the system, I would suggest that we also consider how to deal with similar scenarios in a clustered deployment (in fact many of the production deployment are based on clusters); the simplest use case could be: in a cluster, we have two OFBiz instances connected to the same database; in one instance the list is cached, in the other instance one of the generic values (that are part of the selection) is updated. A distributed cache system may help here.
>>
>> Jacopo
>>
>> On Apr 21, 2013, at 10:54 AM, Adrian Crum <[hidden email]> wrote:
>>
>>> Last week I discovered a flaw in the EntityListCache implementation: http://mail-archives.apache.org/mod_mbox/incubator-ofbiz-dev/201304.mbox/%3C516AC7B4.2020007@...%3E
>>>
>>> To summarize: Entity conditions that are cached become stale when any member of the cached list is changed - making the cache contents invalid. In addition, GenericValues in the cached list are mutable - which is inconsistent with the primary key cache, where GenericValues from the cache are immutable.
>>>
>>> I would like to fix this, but I think we should discuss it first.
>>>
>>> One change would be to make the list member GenericValues immutable. This will make the GenericValues retrieved from the entity list cache consistent with the GenericValues retrieved from the primary key cache, but it won't prevent an invalid cache (because the list member GenericValue can be cloned and modified). Also, this change will likely break a lot of code, because it is common to retrieve a list of GenericValues from the cache and then make changes to the list members. We could create a "transitional" GenericValue that would warn developers when they modify a cached list member, and then switch to an immutable GenericValue some time in the future.
>>>
>>> To fix the stale cache problem, the cache instance can be made a GenericValue listener in all of its list members - so any time a list member is modified the cache will be cleared. This will keep the cache valid, but there might be a performance hit. I'm open to other solutions to this problem.
>>>
>>> Any thoughts?
>>>
>>> -Adrian
>>>
>>>
>

[Adrian Crum-3]

The fix will be pretty simple - when a GenericValue is stored or
removed, any related entity condition caches (the ones the GenericValue
is a member of) will be cleared. So I'm assuming clearing a cache that
is distributed will clear all clustered caches. If you have access to a
test cluster, you can test it after the commit.

-Adrian

On 4/22/2013 10:19 AM, Jacopo Cappellato wrote:
> On Apr 22, 2013, at 10:45 AM, Adrian Crum <[hidden email]> wrote:
>
>> Thanks Jacopo. I haven't looked into the entity cache implementation thoroughly, but I was under the impression that it can be configured to be distributed.
> I don't remember the details but the issue you have discovered for sure also happens in a clustered deployment; and the fix you are proposing would not address the clustered deployment (if I am not missing something).
>
> Jacopo

[Jacques Le Roux]

I guess indeed normally an event will be fired and the cache clearing distributed
I'd ask for a bit of more time for the rest of us to have a deeper look but from your explanation below this seems safe to me.
It would be good if we could measure, or at least have an idea about the order of magnitude on, the performance implications

Thanks

Jacques

From: "Adrian Crum" <[hidden email]>

> The fix will be pretty simple - when a GenericValue is stored or
> removed, any related entity condition caches (the ones the GenericValue
> is a member of) will be cleared. So I'm assuming clearing a cache that
> is distributed will clear all clustered caches. If you have access to a
> test cluster, you can test it after the commit.
>
> -Adrian
>
> On 4/22/2013 10:19 AM, Jacopo Cappellato wrote:
>> On Apr 22, 2013, at 10:45 AM, Adrian Crum <[hidden email]> wrote:
>>
>>> Thanks Jacopo. I haven't looked into the entity cache implementation thoroughly, but I was under the impression that it can be configured to be distributed.
>> I don't remember the details but the issue you have discovered for sure also happens in a clustered deployment; and the fix you are proposing would not address the clustered deployment (if I am not missing something).
>>
>> Jacopo
>

[Jacques Le Roux]

BTW the event is fired by EntityCacheServices.clearCacheLine()

I see that
delegator.clearCacheLineByCondition(entityName, condition, distribute);
is called there, so yes should be safe

Jacques

Jacques Le Roux wrote:

> I guess indeed normally an event will be fired and the cache clearing distributed
> I'd ask for a bit of more time for the rest of us to have a deeper look but from your explanation below this seems safe to me.
> It would be good if we could measure, or at least have an idea about the order of magnitude on, the performance implications
>
> Thanks
>
> Jacques
>
> From: "Adrian Crum" <[hidden email]>
>> The fix will be pretty simple - when a GenericValue is stored or
>> removed, any related entity condition caches (the ones the GenericValue
>> is a member of) will be cleared. So I'm assuming clearing a cache that
>> is distributed will clear all clustered caches. If you have access to a
>> test cluster, you can test it after the commit.
>>
>> -Adrian
>>
>> On 4/22/2013 10:19 AM, Jacopo Cappellato wrote:
>>> On Apr 22, 2013, at 10:45 AM, Adrian Crum <[hidden email]> wrote:
>>>
>>>> Thanks Jacopo. I haven't looked into the entity cache implementation thoroughly, but I was under the impression that it can be
>>>> configured to be distributed.
>>> I don't remember the details but the issue you have discovered for sure also happens in a clustered deployment; and the fix you
>>> are proposing would not address the clustered deployment (if I am not missing something).
>>>
>>> Jacopo

[Jacques Le Roux]

Mmm sorry, rather in this case fired by
EntityCacheServices.distributedClearCacheLineByCondition()

Below is a sub case

Jacques

Jacques Le Roux wrote:

> BTW the event is fired by EntityCacheServices.clearCacheLine()
>
> I see that
> delegator.clearCacheLineByCondition(entityName, condition, distribute);
> is called there, so yes should be safe
>
> Jacques
>
> Jacques Le Roux wrote:
>> I guess indeed normally an event will be fired and the cache clearing distributed
>> I'd ask for a bit of more time for the rest of us to have a deeper look but from your explanation below this seems safe to me.
>> It would be good if we could measure, or at least have an idea about the order of magnitude on, the performance implications
>>
>> Thanks
>>
>> Jacques
>>
>> From: "Adrian Crum" <[hidden email]>
>>> The fix will be pretty simple - when a GenericValue is stored or
>>> removed, any related entity condition caches (the ones the GenericValue
>>> is a member of) will be cleared. So I'm assuming clearing a cache that
>>> is distributed will clear all clustered caches. If you have access to a
>>> test cluster, you can test it after the commit.
>>>
>>> -Adrian
>>>
>>> On 4/22/2013 10:19 AM, Jacopo Cappellato wrote:
>>>> On Apr 22, 2013, at 10:45 AM, Adrian Crum <[hidden email]> wrote:
>>>>
>>>>> Thanks Jacopo. I haven't looked into the entity cache implementation thoroughly, but I was under the impression that it can be
>>>>> configured to be distributed.
>>>> I don't remember the details but the issue you have discovered for sure also happens in a clustered deployment; and the fix you
>>>> are proposing would not address the clustered deployment (if I am not missing something).
>>>>
>>>> Jacopo

[Adrian Crum-3]

The performance hit I envisioned was the overhead of notifying listeners
during store or remove. But I think I solved that with the custom
Observable class and some other optimizations I have in mind.

-Adrian

On 4/22/2013 11:02 AM, Jacques Le Roux wrote:

> I guess indeed normally an event will be fired and the cache clearing distributed
> I'd ask for a bit of more time for the rest of us to have a deeper look but from your explanation below this seems safe to me.
> It would be good if we could measure, or at least have an idea about the order of magnitude on, the performance implications
>
> Thanks
>
> Jacques
>
> From: "Adrian Crum" <[hidden email]>
>> The fix will be pretty simple - when a GenericValue is stored or
>> removed, any related entity condition caches (the ones the GenericValue
>> is a member of) will be cleared. So I'm assuming clearing a cache that
>> is distributed will clear all clustered caches. If you have access to a
>> test cluster, you can test it after the commit.
>>
>> -Adrian
>>
>> On 4/22/2013 10:19 AM, Jacopo Cappellato wrote:
>>> On Apr 22, 2013, at 10:45 AM, Adrian Crum <[hidden email]> wrote:
>>>
>>>> Thanks Jacopo. I haven't looked into the entity cache implementation thoroughly, but I was under the impression that it can be configured to be distributed.
>>> I don't remember the details but the issue you have discovered for sure also happens in a clustered deployment; and the fix you are proposing would not address the clustered deployment (if I am not missing something).
>>>
>>> Jacopo

[Adrian Crum-3]

I went ahead and committed the immutable GenericEntity change. I didn't
want to wait or come up with a "transitional" GenericValue. After
looking through the code, I determined that leaving cached instances
mutable could cause data corruption.

I still plan to commit the stale cache fix this weekend.

-Adrian

On 4/22/2013 9:45 AM, Adrian Crum wrote:

> Thanks Jacopo. I haven't looked into the entity cache implementation
> thoroughly, but I was under the impression that it can be configured
> to be distributed.
>
> I have the fix working on my local copy. As you can see, I have made
> some related changes already and will be making some more, but I won't
> commit the fix until next weekend - to give everyone a chance to respond.
>
> -Adrian
>
> On 4/22/2013 9:33 AM, Jacopo Cappellato wrote:
>> It seems actually to be an issue rather than a feature (I can't think
>> of a use case where this behavior would be useful); I have created a
>> few test cases (similar to the one you have provided in the other
>> thread) that further analyze your discovery but they don't add much
>> to what you found (apart from confirming the risk of getting stale
>> data).
>> However, when we start design/implement a refactoring of this part of
>> the system, I would suggest that we also consider how to deal with
>> similar scenarios in a clustered deployment (in fact many of the
>> production deployment are based on clusters); the simplest use case
>> could be: in a cluster, we have two OFBiz instances connected to the
>> same database; in one instance the list is cached, in the other
>> instance one of the generic values (that are part of the selection)
>> is updated. A distributed cache system may help here.
>>
>> Jacopo
>>
>> On Apr 21, 2013, at 10:54 AM, Adrian Crum
>> <[hidden email]> wrote:
>>
>>> Last week I discovered a flaw in the EntityListCache implementation:
>>> http://mail-archives.apache.org/mod_mbox/incubator-ofbiz-dev/201304.mbox/%3C516AC7B4.2020007@...%3E
>>>
>>> To summarize: Entity conditions that are cached become stale when
>>> any member of the cached list is changed - making the cache contents
>>> invalid. In addition, GenericValues in the cached list are mutable -
>>> which is inconsistent with the primary key cache, where
>>> GenericValues from the cache are immutable.
>>>
>>> I would like to fix this, but I think we should discuss it first.
>>>
>>> One change would be to make the list member GenericValues immutable.
>>> This will make the GenericValues retrieved from the entity list
>>> cache consistent with the GenericValues retrieved from the primary
>>> key cache, but it won't prevent an invalid cache (because the list
>>> member GenericValue can be cloned and modified). Also, this change
>>> will likely break a lot of code, because it is common to retrieve a
>>> list of GenericValues from the cache and then make changes to the
>>> list members. We could create a "transitional" GenericValue that
>>> would warn developers when they modify a cached list member, and
>>> then switch to an immutable GenericValue some time in the future.
>>>
>>> To fix the stale cache problem, the cache instance can be made a
>>> GenericValue listener in all of its list members - so any time a
>>> list member is modified the cache will be cleared. This will keep
>>> the cache valid, but there might be a performance hit. I'm open to
>>> other solutions to this problem.
>>>
>>> Any thoughts?
>>>
>>> -Adrian
>>>
>>>
>

[Adrian Crum-3]

I fixed the entity cache issues in revision 1476296. The fix does not
include the distributed cache system, but that should be easy to fix by
duplicating the fixes to the local cache.

I found some flaws in the entity engine that I will discuss in another
thread.

-Adrian

On 4/22/2013 9:45 AM, Adrian Crum wrote:

> Thanks Jacopo. I haven't looked into the entity cache implementation
> thoroughly, but I was under the impression that it can be configured
> to be distributed.
>
> I have the fix working on my local copy. As you can see, I have made
> some related changes already and will be making some more, but I won't
> commit the fix until next weekend - to give everyone a chance to respond.
>
> -Adrian
>
> On 4/22/2013 9:33 AM, Jacopo Cappellato wrote:
>> It seems actually to be an issue rather than a feature (I can't think
>> of a use case where this behavior would be useful); I have created a
>> few test cases (similar to the one you have provided in the other
>> thread) that further analyze your discovery but they don't add much
>> to what you found (apart from confirming the risk of getting stale
>> data).
>> However, when we start design/implement a refactoring of this part of
>> the system, I would suggest that we also consider how to deal with
>> similar scenarios in a clustered deployment (in fact many of the
>> production deployment are based on clusters); the simplest use case
>> could be: in a cluster, we have two OFBiz instances connected to the
>> same database; in one instance the list is cached, in the other
>> instance one of the generic values (that are part of the selection)
>> is updated. A distributed cache system may help here.
>>
>> Jacopo
>>
>> On Apr 21, 2013, at 10:54 AM, Adrian Crum
>> <[hidden email]> wrote:
>>
>>> Last week I discovered a flaw in the EntityListCache implementation:
>>> http://mail-archives.apache.org/mod_mbox/incubator-ofbiz-dev/201304.mbox/%3C516AC7B4.2020007@...%3E
>>>
>>> To summarize: Entity conditions that are cached become stale when
>>> any member of the cached list is changed - making the cache contents
>>> invalid. In addition, GenericValues in the cached list are mutable -
>>> which is inconsistent with the primary key cache, where
>>> GenericValues from the cache are immutable.
>>>
>>> I would like to fix this, but I think we should discuss it first.
>>>
>>> One change would be to make the list member GenericValues immutable.
>>> This will make the GenericValues retrieved from the entity list
>>> cache consistent with the GenericValues retrieved from the primary
>>> key cache, but it won't prevent an invalid cache (because the list
>>> member GenericValue can be cloned and modified). Also, this change
>>> will likely break a lot of code, because it is common to retrieve a
>>> list of GenericValues from the cache and then make changes to the
>>> list members. We could create a "transitional" GenericValue that
>>> would warn developers when they modify a cached list member, and
>>> then switch to an immutable GenericValue some time in the future.
>>>
>>> To fix the stale cache problem, the cache instance can be made a
>>> GenericValue listener in all of its list members - so any time a
>>> list member is modified the cache will be cleared. This will keep
>>> the cache valid, but there might be a performance hit. I'm open to
>>> other solutions to this problem.
>>>
>>> Any thoughts?
>>>
>>> -Adrian
>>>
>>>
>

[Jacques Le Roux]

Hi Adrian,

Did you also fix the DCC since?

Jacques

From: "Adrian Crum" <[hidden email]>

>I fixed the entity cache issues in revision 1476296. The fix does not
> include the distributed cache system, but that should be easy to fix by
> duplicating the fixes to the local cache.
>
> I found some flaws in the entity engine that I will discuss in another
> thread.
>
> -Adrian
>
> On 4/22/2013 9:45 AM, Adrian Crum wrote:
>> Thanks Jacopo. I haven't looked into the entity cache implementation
>> thoroughly, but I was under the impression that it can be configured
>> to be distributed.
>>
>> I have the fix working on my local copy. As you can see, I have made
>> some related changes already and will be making some more, but I won't
>> commit the fix until next weekend - to give everyone a chance to respond.
>>
>> -Adrian
>>
>> On 4/22/2013 9:33 AM, Jacopo Cappellato wrote:
>>> It seems actually to be an issue rather than a feature (I can't think
>>> of a use case where this behavior would be useful); I have created a
>>> few test cases (similar to the one you have provided in the other
>>> thread) that further analyze your discovery but they don't add much
>>> to what you found (apart from confirming the risk of getting stale
>>> data).
>>> However, when we start design/implement a refactoring of this part of
>>> the system, I would suggest that we also consider how to deal with
>>> similar scenarios in a clustered deployment (in fact many of the
>>> production deployment are based on clusters); the simplest use case
>>> could be: in a cluster, we have two OFBiz instances connected to the
>>> same database; in one instance the list is cached, in the other
>>> instance one of the generic values (that are part of the selection)
>>> is updated. A distributed cache system may help here.
>>>
>>> Jacopo
>>>
>>> On Apr 21, 2013, at 10:54 AM, Adrian Crum
>>> <[hidden email]> wrote:
>>>
>>>> Last week I discovered a flaw in the EntityListCache implementation:
>>>> http://mail-archives.apache.org/mod_mbox/incubator-ofbiz-dev/201304.mbox/%3C516AC7B4.2020007@...%3E
>>>>
>>>> To summarize: Entity conditions that are cached become stale when
>>>> any member of the cached list is changed - making the cache contents
>>>> invalid. In addition, GenericValues in the cached list are mutable -
>>>> which is inconsistent with the primary key cache, where
>>>> GenericValues from the cache are immutable.
>>>>
>>>> I would like to fix this, but I think we should discuss it first.
>>>>
>>>> One change would be to make the list member GenericValues immutable.
>>>> This will make the GenericValues retrieved from the entity list
>>>> cache consistent with the GenericValues retrieved from the primary
>>>> key cache, but it won't prevent an invalid cache (because the list
>>>> member GenericValue can be cloned and modified). Also, this change
>>>> will likely break a lot of code, because it is common to retrieve a
>>>> list of GenericValues from the cache and then make changes to the
>>>> list members. We could create a "transitional" GenericValue that
>>>> would warn developers when they modify a cached list member, and
>>>> then switch to an immutable GenericValue some time in the future.
>>>>
>>>> To fix the stale cache problem, the cache instance can be made a
>>>> GenericValue listener in all of its list members - so any time a
>>>> list member is modified the cache will be cleared. This will keep
>>>> the cache valid, but there might be a performance hit. I'm open to
>>>> other solutions to this problem.
>>>>
>>>> Any thoughts?
>>>>
>>>> -Adrian
>>>>
>>>>
>>
>

[Adrian Crum-3]

No, I didn't get to that yet. I found problems in the entity models, so
I'm currently detoured to fix those, then I will get back to the
GenericEntity and caching issues.

-Adrian

On 5/4/2013 8:25 AM, Jacques Le Roux wrote:

> Hi Adrian,
>
> Did you also fix the DCC since?
>
> Jacques
>
> From: "Adrian Crum" <[hidden email]>
>> I fixed the entity cache issues in revision 1476296. The fix does not
>> include the distributed cache system, but that should be easy to fix by
>> duplicating the fixes to the local cache.
>>
>> I found some flaws in the entity engine that I will discuss in another
>> thread.
>>
>> -Adrian
>>
>> On 4/22/2013 9:45 AM, Adrian Crum wrote:
>>> Thanks Jacopo. I haven't looked into the entity cache implementation
>>> thoroughly, but I was under the impression that it can be configured
>>> to be distributed.
>>>
>>> I have the fix working on my local copy. As you can see, I have made
>>> some related changes already and will be making some more, but I won't
>>> commit the fix until next weekend - to give everyone a chance to respond.
>>>
>>> -Adrian
>>>
>>> On 4/22/2013 9:33 AM, Jacopo Cappellato wrote:
>>>> It seems actually to be an issue rather than a feature (I can't think
>>>> of a use case where this behavior would be useful); I have created a
>>>> few test cases (similar to the one you have provided in the other
>>>> thread) that further analyze your discovery but they don't add much
>>>> to what you found (apart from confirming the risk of getting stale
>>>> data).
>>>> However, when we start design/implement a refactoring of this part of
>>>> the system, I would suggest that we also consider how to deal with
>>>> similar scenarios in a clustered deployment (in fact many of the
>>>> production deployment are based on clusters); the simplest use case
>>>> could be: in a cluster, we have two OFBiz instances connected to the
>>>> same database; in one instance the list is cached, in the other
>>>> instance one of the generic values (that are part of the selection)
>>>> is updated. A distributed cache system may help here.
>>>>
>>>> Jacopo
>>>>
>>>> On Apr 21, 2013, at 10:54 AM, Adrian Crum
>>>> <[hidden email]> wrote:
>>>>
>>>>> Last week I discovered a flaw in the EntityListCache implementation:
>>>>> http://mail-archives.apache.org/mod_mbox/incubator-ofbiz-dev/201304.mbox/%3C516AC7B4.2020007@...%3E
>>>>>
>>>>> To summarize: Entity conditions that are cached become stale when
>>>>> any member of the cached list is changed - making the cache contents
>>>>> invalid. In addition, GenericValues in the cached list are mutable -
>>>>> which is inconsistent with the primary key cache, where
>>>>> GenericValues from the cache are immutable.
>>>>>
>>>>> I would like to fix this, but I think we should discuss it first.
>>>>>
>>>>> One change would be to make the list member GenericValues immutable.
>>>>> This will make the GenericValues retrieved from the entity list
>>>>> cache consistent with the GenericValues retrieved from the primary
>>>>> key cache, but it won't prevent an invalid cache (because the list
>>>>> member GenericValue can be cloned and modified). Also, this change
>>>>> will likely break a lot of code, because it is common to retrieve a
>>>>> list of GenericValues from the cache and then make changes to the
>>>>> list members. We could create a "transitional" GenericValue that
>>>>> would warn developers when they modify a cached list member, and
>>>>> then switch to an immutable GenericValue some time in the future.
>>>>>
>>>>> To fix the stale cache problem, the cache instance can be made a
>>>>> GenericValue listener in all of its list members - so any time a
>>>>> list member is modified the cache will be cleared. This will keep
>>>>> the cache valid, but there might be a performance hit. I'm open to
>>>>> other solutions to this problem.
>>>>>
>>>>> Any thoughts?
>>>>>
>>>>> -Adrian
>>>>>
>>>>>

[Jacques Le Roux]

Thanks for your work and feedback Adrian!

I already did cursorily reviewed your work, if I get some chances to review more in details, which revisions will you recommend?

Jacques

From: "Adrian Crum" <[hidden email]>

> No, I didn't get to that yet. I found problems in the entity models, so
> I'm currently detoured to fix those, then I will get back to the
> GenericEntity and caching issues.
>
> -Adrian
>
> On 5/4/2013 8:25 AM, Jacques Le Roux wrote:
>> Hi Adrian,
>>
>> Did you also fix the DCC since?
>>
>> Jacques
>>
>> From: "Adrian Crum" <[hidden email]>
>>> I fixed the entity cache issues in revision 1476296. The fix does not
>>> include the distributed cache system, but that should be easy to fix by
>>> duplicating the fixes to the local cache.
>>>
>>> I found some flaws in the entity engine that I will discuss in another
>>> thread.
>>>
>>> -Adrian
>>>
>>> On 4/22/2013 9:45 AM, Adrian Crum wrote:
>>>> Thanks Jacopo. I haven't looked into the entity cache implementation
>>>> thoroughly, but I was under the impression that it can be configured
>>>> to be distributed.
>>>>
>>>> I have the fix working on my local copy. As you can see, I have made
>>>> some related changes already and will be making some more, but I won't
>>>> commit the fix until next weekend - to give everyone a chance to respond.
>>>>
>>>> -Adrian
>>>>
>>>> On 4/22/2013 9:33 AM, Jacopo Cappellato wrote:
>>>>> It seems actually to be an issue rather than a feature (I can't think
>>>>> of a use case where this behavior would be useful); I have created a
>>>>> few test cases (similar to the one you have provided in the other
>>>>> thread) that further analyze your discovery but they don't add much
>>>>> to what you found (apart from confirming the risk of getting stale
>>>>> data).
>>>>> However, when we start design/implement a refactoring of this part of
>>>>> the system, I would suggest that we also consider how to deal with
>>>>> similar scenarios in a clustered deployment (in fact many of the
>>>>> production deployment are based on clusters); the simplest use case
>>>>> could be: in a cluster, we have two OFBiz instances connected to the
>>>>> same database; in one instance the list is cached, in the other
>>>>> instance one of the generic values (that are part of the selection)
>>>>> is updated. A distributed cache system may help here.
>>>>>
>>>>> Jacopo
>>>>>
>>>>> On Apr 21, 2013, at 10:54 AM, Adrian Crum
>>>>> <[hidden email]> wrote:
>>>>>
>>>>>> Last week I discovered a flaw in the EntityListCache implementation:
>>>>>> http://mail-archives.apache.org/mod_mbox/incubator-ofbiz-dev/201304.mbox/%3C516AC7B4.2020007@...%3E
>>>>>>
>>>>>> To summarize: Entity conditions that are cached become stale when
>>>>>> any member of the cached list is changed - making the cache contents
>>>>>> invalid. In addition, GenericValues in the cached list are mutable -
>>>>>> which is inconsistent with the primary key cache, where
>>>>>> GenericValues from the cache are immutable.
>>>>>>
>>>>>> I would like to fix this, but I think we should discuss it first.
>>>>>>
>>>>>> One change would be to make the list member GenericValues immutable.
>>>>>> This will make the GenericValues retrieved from the entity list
>>>>>> cache consistent with the GenericValues retrieved from the primary
>>>>>> key cache, but it won't prevent an invalid cache (because the list
>>>>>> member GenericValue can be cloned and modified). Also, this change
>>>>>> will likely break a lot of code, because it is common to retrieve a
>>>>>> list of GenericValues from the cache and then make changes to the
>>>>>> list members. We could create a "transitional" GenericValue that
>>>>>> would warn developers when they modify a cached list member, and
>>>>>> then switch to an immutable GenericValue some time in the future.
>>>>>>
>>>>>> To fix the stale cache problem, the cache instance can be made a
>>>>>> GenericValue listener in all of its list members - so any time a
>>>>>> list member is modified the cache will be cleared. This will keep
>>>>>> the cache valid, but there might be a performance hit. I'm open to
>>>>>> other solutions to this problem.
>>>>>>
>>>>>> Any thoughts?
>>>>>>
>>>>>> -Adrian
>>>>>>
>>>>>>
>

[Adrian Crum-3]

Revision 1476296.

-Adrian

On 5/4/2013 8:42 AM, Jacques Le Roux wrote:

> Thanks for your work and feedback Adrian!
>
> I already did cursorily reviewed your work, if I get some chances to review more in details, which revisions will you recommend?
>
> Jacques
>
> From: "Adrian Crum" <[hidden email]>
>> No, I didn't get to that yet. I found problems in the entity models, so
>> I'm currently detoured to fix those, then I will get back to the
>> GenericEntity and caching issues.
>>
>> -Adrian
>>
>> On 5/4/2013 8:25 AM, Jacques Le Roux wrote:
>>> Hi Adrian,
>>>
>>> Did you also fix the DCC since?
>>>
>>> Jacques
>>>
>>> From: "Adrian Crum" <[hidden email]>
>>>> I fixed the entity cache issues in revision 1476296. The fix does not
>>>> include the distributed cache system, but that should be easy to fix by
>>>> duplicating the fixes to the local cache.
>>>>
>>>> I found some flaws in the entity engine that I will discuss in another
>>>> thread.
>>>>
>>>> -Adrian
>>>>
>>>> On 4/22/2013 9:45 AM, Adrian Crum wrote:
>>>>> Thanks Jacopo. I haven't looked into the entity cache implementation
>>>>> thoroughly, but I was under the impression that it can be configured
>>>>> to be distributed.
>>>>>
>>>>> I have the fix working on my local copy. As you can see, I have made
>>>>> some related changes already and will be making some more, but I won't
>>>>> commit the fix until next weekend - to give everyone a chance to respond.
>>>>>
>>>>> -Adrian
>>>>>
>>>>> On 4/22/2013 9:33 AM, Jacopo Cappellato wrote:
>>>>>> It seems actually to be an issue rather than a feature (I can't think
>>>>>> of a use case where this behavior would be useful); I have created a
>>>>>> few test cases (similar to the one you have provided in the other
>>>>>> thread) that further analyze your discovery but they don't add much
>>>>>> to what you found (apart from confirming the risk of getting stale
>>>>>> data).
>>>>>> However, when we start design/implement a refactoring of this part of
>>>>>> the system, I would suggest that we also consider how to deal with
>>>>>> similar scenarios in a clustered deployment (in fact many of the
>>>>>> production deployment are based on clusters); the simplest use case
>>>>>> could be: in a cluster, we have two OFBiz instances connected to the
>>>>>> same database; in one instance the list is cached, in the other
>>>>>> instance one of the generic values (that are part of the selection)
>>>>>> is updated. A distributed cache system may help here.
>>>>>>
>>>>>> Jacopo
>>>>>>
>>>>>> On Apr 21, 2013, at 10:54 AM, Adrian Crum
>>>>>> <[hidden email]> wrote:
>>>>>>
>>>>>>> Last week I discovered a flaw in the EntityListCache implementation:
>>>>>>> http://mail-archives.apache.org/mod_mbox/incubator-ofbiz-dev/201304.mbox/%3C516AC7B4.2020007@...%3E
>>>>>>>
>>>>>>> To summarize: Entity conditions that are cached become stale when
>>>>>>> any member of the cached list is changed - making the cache contents
>>>>>>> invalid. In addition, GenericValues in the cached list are mutable -
>>>>>>> which is inconsistent with the primary key cache, where
>>>>>>> GenericValues from the cache are immutable.
>>>>>>>
>>>>>>> I would like to fix this, but I think we should discuss it first.
>>>>>>>
>>>>>>> One change would be to make the list member GenericValues immutable.
>>>>>>> This will make the GenericValues retrieved from the entity list
>>>>>>> cache consistent with the GenericValues retrieved from the primary
>>>>>>> key cache, but it won't prevent an invalid cache (because the list
>>>>>>> member GenericValue can be cloned and modified). Also, this change
>>>>>>> will likely break a lot of code, because it is common to retrieve a
>>>>>>> list of GenericValues from the cache and then make changes to the
>>>>>>> list members. We could create a "transitional" GenericValue that
>>>>>>> would warn developers when they modify a cached list member, and
>>>>>>> then switch to an immutable GenericValue some time in the future.
>>>>>>>
>>>>>>> To fix the stale cache problem, the cache instance can be made a
>>>>>>> GenericValue listener in all of its list members - so any time a
>>>>>>> list member is modified the cache will be cleared. This will keep
>>>>>>> the cache valid, but there might be a performance hit. I'm open to
>>>>>>> other solutions to this problem.
>>>>>>>
>>>>>>> Any thoughts?
>>>>>>>
>>>>>>> -Adrian
>>>>>>>
>>>>>>>

[Jacques Le Roux]

What makes you think changes are needed for the DCC?

Jacques

From: "Adrian Crum" <[hidden email]>

>I fixed the entity cache issues in revision 1476296. The fix does not
> include the distributed cache system, but that should be easy to fix by
> duplicating the fixes to the local cache.
>
> I found some flaws in the entity engine that I will discuss in another
> thread.
>
> -Adrian
>
> On 4/22/2013 9:45 AM, Adrian Crum wrote:
>> Thanks Jacopo. I haven't looked into the entity cache implementation
>> thoroughly, but I was under the impression that it can be configured
>> to be distributed.
>>
>> I have the fix working on my local copy. As you can see, I have made
>> some related changes already and will be making some more, but I won't
>> commit the fix until next weekend - to give everyone a chance to respond.
>>
>> -Adrian
>>
>> On 4/22/2013 9:33 AM, Jacopo Cappellato wrote:
>>> It seems actually to be an issue rather than a feature (I can't think
>>> of a use case where this behavior would be useful); I have created a
>>> few test cases (similar to the one you have provided in the other
>>> thread) that further analyze your discovery but they don't add much
>>> to what you found (apart from confirming the risk of getting stale
>>> data).
>>> However, when we start design/implement a refactoring of this part of
>>> the system, I would suggest that we also consider how to deal with
>>> similar scenarios in a clustered deployment (in fact many of the
>>> production deployment are based on clusters); the simplest use case
>>> could be: in a cluster, we have two OFBiz instances connected to the
>>> same database; in one instance the list is cached, in the other
>>> instance one of the generic values (that are part of the selection)
>>> is updated. A distributed cache system may help here.
>>>
>>> Jacopo
>>>
>>> On Apr 21, 2013, at 10:54 AM, Adrian Crum
>>> <[hidden email]> wrote:
>>>
>>>> Last week I discovered a flaw in the EntityListCache implementation:
>>>> http://mail-archives.apache.org/mod_mbox/incubator-ofbiz-dev/201304.mbox/%3C516AC7B4.2020007@...%3E
>>>>
>>>> To summarize: Entity conditions that are cached become stale when
>>>> any member of the cached list is changed - making the cache contents
>>>> invalid. In addition, GenericValues in the cached list are mutable -
>>>> which is inconsistent with the primary key cache, where
>>>> GenericValues from the cache are immutable.
>>>>
>>>> I would like to fix this, but I think we should discuss it first.
>>>>
>>>> One change would be to make the list member GenericValues immutable.
>>>> This will make the GenericValues retrieved from the entity list
>>>> cache consistent with the GenericValues retrieved from the primary
>>>> key cache, but it won't prevent an invalid cache (because the list
>>>> member GenericValue can be cloned and modified). Also, this change
>>>> will likely break a lot of code, because it is common to retrieve a
>>>> list of GenericValues from the cache and then make changes to the
>>>> list members. We could create a "transitional" GenericValue that
>>>> would warn developers when they modify a cached list member, and
>>>> then switch to an immutable GenericValue some time in the future.
>>>>
>>>> To fix the stale cache problem, the cache instance can be made a
>>>> GenericValue listener in all of its list members - so any time a
>>>> list member is modified the cache will be cleared. This will keep
>>>> the cache valid, but there might be a performance hit. I'm open to
>>>> other solutions to this problem.
>>>>
>>>> Any thoughts?
>>>>
>>>> -Adrian
>>>>
>>>>
>>
>

[Adrian Crum-3]

I don't know if any changes are necessary for DCC. I was assuming
something needs to be fixed.

-Adrian

On 6/1/2013 10:36 PM, Jacques Le Roux wrote:

> What makes you think changes are needed for the DCC?
>
> Jacques
>
> From: "Adrian Crum" <[hidden email]>
>> I fixed the entity cache issues in revision 1476296. The fix does not
>> include the distributed cache system, but that should be easy to fix by
>> duplicating the fixes to the local cache.
>>
>> I found some flaws in the entity engine that I will discuss in another
>> thread.
>>
>> -Adrian
>>
>> On 4/22/2013 9:45 AM, Adrian Crum wrote:
>>> Thanks Jacopo. I haven't looked into the entity cache implementation
>>> thoroughly, but I was under the impression that it can be configured
>>> to be distributed.
>>>
>>> I have the fix working on my local copy. As you can see, I have made
>>> some related changes already and will be making some more, but I won't
>>> commit the fix until next weekend - to give everyone a chance to respond.
>>>
>>> -Adrian
>>>
>>> On 4/22/2013 9:33 AM, Jacopo Cappellato wrote:
>>>> It seems actually to be an issue rather than a feature (I can't think
>>>> of a use case where this behavior would be useful); I have created a
>>>> few test cases (similar to the one you have provided in the other
>>>> thread) that further analyze your discovery but they don't add much
>>>> to what you found (apart from confirming the risk of getting stale
>>>> data).
>>>> However, when we start design/implement a refactoring of this part of
>>>> the system, I would suggest that we also consider how to deal with
>>>> similar scenarios in a clustered deployment (in fact many of the
>>>> production deployment are based on clusters); the simplest use case
>>>> could be: in a cluster, we have two OFBiz instances connected to the
>>>> same database; in one instance the list is cached, in the other
>>>> instance one of the generic values (that are part of the selection)
>>>> is updated. A distributed cache system may help here.
>>>>
>>>> Jacopo
>>>>
>>>> On Apr 21, 2013, at 10:54 AM, Adrian Crum
>>>> <[hidden email]> wrote:
>>>>
>>>>> Last week I discovered a flaw in the EntityListCache implementation:
>>>>> http://mail-archives.apache.org/mod_mbox/incubator-ofbiz-dev/201304.mbox/%3C516AC7B4.2020007@...%3E
>>>>>
>>>>> To summarize: Entity conditions that are cached become stale when
>>>>> any member of the cached list is changed - making the cache contents
>>>>> invalid. In addition, GenericValues in the cached list are mutable -
>>>>> which is inconsistent with the primary key cache, where
>>>>> GenericValues from the cache are immutable.
>>>>>
>>>>> I would like to fix this, but I think we should discuss it first.
>>>>>
>>>>> One change would be to make the list member GenericValues immutable.
>>>>> This will make the GenericValues retrieved from the entity list
>>>>> cache consistent with the GenericValues retrieved from the primary
>>>>> key cache, but it won't prevent an invalid cache (because the list
>>>>> member GenericValue can be cloned and modified). Also, this change
>>>>> will likely break a lot of code, because it is common to retrieve a
>>>>> list of GenericValues from the cache and then make changes to the
>>>>> list members. We could create a "transitional" GenericValue that
>>>>> would warn developers when they modify a cached list member, and
>>>>> then switch to an immutable GenericValue some time in the future.
>>>>>
>>>>> To fix the stale cache problem, the cache instance can be made a
>>>>> GenericValue listener in all of its list members - so any time a
>>>>> list member is modified the cache will be cleared. This will keep
>>>>> the cache valid, but there might be a performance hit. I'm open to
>>>>> other solutions to this problem.
>>>>>
>>>>> Any thoughts?
>>>>>
>>>>> -Adrian
>>>>>
>>>>>

[Jacques Le Roux]

I will double check, from what I already know and re-read recently nothing should be needed

Jacques

From: "Adrian Crum" <[hidden email]>

>I don't know if any changes are necessary for DCC. I was assuming
> something needs to be fixed.
>
> -Adrian
>
> On 6/1/2013 10:36 PM, Jacques Le Roux wrote:
>> What makes you think changes are needed for the DCC?
>>
>> Jacques
>>
>> From: "Adrian Crum" <[hidden email]>
>>> I fixed the entity cache issues in revision 1476296. The fix does not
>>> include the distributed cache system, but that should be easy to fix by
>>> duplicating the fixes to the local cache.
>>>
>>> I found some flaws in the entity engine that I will discuss in another
>>> thread.
>>>
>>> -Adrian
>>>
>>> On 4/22/2013 9:45 AM, Adrian Crum wrote:
>>>> Thanks Jacopo. I haven't looked into the entity cache implementation
>>>> thoroughly, but I was under the impression that it can be configured
>>>> to be distributed.
>>>>
>>>> I have the fix working on my local copy. As you can see, I have made
>>>> some related changes already and will be making some more, but I won't
>>>> commit the fix until next weekend - to give everyone a chance to respond.
>>>>
>>>> -Adrian
>>>>
>>>> On 4/22/2013 9:33 AM, Jacopo Cappellato wrote:
>>>>> It seems actually to be an issue rather than a feature (I can't think
>>>>> of a use case where this behavior would be useful); I have created a
>>>>> few test cases (similar to the one you have provided in the other
>>>>> thread) that further analyze your discovery but they don't add much
>>>>> to what you found (apart from confirming the risk of getting stale
>>>>> data).
>>>>> However, when we start design/implement a refactoring of this part of
>>>>> the system, I would suggest that we also consider how to deal with
>>>>> similar scenarios in a clustered deployment (in fact many of the
>>>>> production deployment are based on clusters); the simplest use case
>>>>> could be: in a cluster, we have two OFBiz instances connected to the
>>>>> same database; in one instance the list is cached, in the other
>>>>> instance one of the generic values (that are part of the selection)
>>>>> is updated. A distributed cache system may help here.
>>>>>
>>>>> Jacopo
>>>>>
>>>>> On Apr 21, 2013, at 10:54 AM, Adrian Crum
>>>>> <[hidden email]> wrote:
>>>>>
>>>>>> Last week I discovered a flaw in the EntityListCache implementation:
>>>>>> http://mail-archives.apache.org/mod_mbox/incubator-ofbiz-dev/201304.mbox/%3C516AC7B4.2020007@...%3E
>>>>>>
>>>>>> To summarize: Entity conditions that are cached become stale when
>>>>>> any member of the cached list is changed - making the cache contents
>>>>>> invalid. In addition, GenericValues in the cached list are mutable -
>>>>>> which is inconsistent with the primary key cache, where
>>>>>> GenericValues from the cache are immutable.
>>>>>>
>>>>>> I would like to fix this, but I think we should discuss it first.
>>>>>>
>>>>>> One change would be to make the list member GenericValues immutable.
>>>>>> This will make the GenericValues retrieved from the entity list
>>>>>> cache consistent with the GenericValues retrieved from the primary
>>>>>> key cache, but it won't prevent an invalid cache (because the list
>>>>>> member GenericValue can be cloned and modified). Also, this change
>>>>>> will likely break a lot of code, because it is common to retrieve a
>>>>>> list of GenericValues from the cache and then make changes to the
>>>>>> list members. We could create a "transitional" GenericValue that
>>>>>> would warn developers when they modify a cached list member, and
>>>>>> then switch to an immutable GenericValue some time in the future.
>>>>>>
>>>>>> To fix the stale cache problem, the cache instance can be made a
>>>>>> GenericValue listener in all of its list members - so any time a
>>>>>> list member is modified the cache will be cleared. This will keep
>>>>>> the cache valid, but there might be a performance hit. I'm open to
>>>>>> other solutions to this problem.
>>>>>>
>>>>>> Any thoughts?
>>>>>>
>>>>>> -Adrian
>>>>>>
>>>>>>
>