polygene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Tibor Mlynarik <tibor.mlyna...@gmail.com>
Subject Re: JSONEntityState internal clone logic
Date Fri, 24 Jul 2015 05:06:39 GMT
yes, test in pull request.

cheers, 

	Tibor


On Jul 24, 2015, at 7:01 AM, Niclas Hedhman <niclas@hedhman.org> wrote:

> The "this testcase" is referring to the pull request? I'll take a look a
> bit later.... Checking the "Release" at the moment.
> 
> Cheers
> Niclas
> 
> On Fri, Jul 24, 2015 at 12:14 PM, Tibor Mlynarik <tibor.mlynarik@gmail.com>
> wrote:
> 
>> Hi Niclas,
>> 
>>> 
>>> The "isStateNotCloned()" fix seems legit and a testcase for that would be
>>> awesome (can't figure one out in my at this early hour)
>> 
>> this testcase should be showing that "isStateNotCloned()" is broken.
>> Change to entity in uow3, which is discarded, is visible to others, as
>> cloning is not working.
>> 
>> account3.balance().set( BigDecimal.TEN);
>> uow3.discard() ;
>> 
>> on other side, clone is executed with each set to entity, what is probably
>> only unnecessary performance penalty.
>> 
>>> As for the testcase, the bug that it exposes is that it doesn't prevent
>> you
>>> from using the "Account account1" references outside the UoW it was
>> created
>>> in.
>> 
>> 
>> I see your point that accessing entity fetched from other uow ( closed
>> one) should be prohibited.
>> But maybe accessing just identity value should be allowed.
>> But this is not what I wanted to report.
>> 
>> thanks ,
>> 
>>        Tibor
>> 
>> On Jul 24, 2015, at 3:50 AM, Niclas Hedhman <niclas@hedhman.org> wrote:
>> 
>>> I have not checked the code yet, but the clone is supposed to be there to
>>> provide UoW isolation, i.e. a value pulled out of the cache, and modified
>>> (any of the set methods) must not be visible into the cached state until
>>> the UoW completes, at which time I presume the entity state is replaced
>> in
>>> the the cache.
>>> 
>>> As for the testcase, the bug that it exposes is that it doesn't prevent
>> you
>>> from using the "Account account1" references outside the UoW it was
>> created
>>> in. That is the bug, and I would be happy to look into that (perhaps for
>>> 3.0 just in case you depend on this fault behavior). If identity is used,
>>> it is more likely working as expected.
>>> 
>>> 
>>> The "isStateNotCloned()" fix seems legit and a testcase for that would be
>>> awesome (can't figure one out in my at this early hour)
>>> 
>>> 
>>> Thanks, Tibor
>>> 
>>> 
>>> On Fri, Jul 24, 2015 at 12:31 AM, Tibor Mlynarik <
>> tibor.mlynarik@gmail.com>
>>> wrote:
>>> 
>>>> Hi Paul,
>>>> 
>>>> I have prepared test case to demonstrate issue.
>>>> Problem is if there is enabled cache in json based entity store.
>>>> Entities could be in wrong state.
>>>> 
>>>> see here :
>>>> https://github.com/apache/zest-qi4j/pull/2
>>>> 
>>>> to make test success, fix :
>>>> 
>>>> JSONEntityState
>>>>   void cloneStateIfGlobalStateLoaded()
>>>>   {
>>>>       if( ! isStateNotCloned() )
>>>> 
>>>> 
>>>> there is one more fix needed in JSONMapEntityStoreMixin#fetchCachedState
>>>> method .
>>>> 
>>>> Note: I was not able to configure entity store with cache service in one
>>>> layer ,
>>>> probably because of circular dependency: store uses cache, cache need
>>>> store for config .
>>>> 
>>>> cheers,
>>>> 
>>>>       Tibor
>>>> 
>>>> 
>>>> On Jul 23, 2015, at 1:49 PM, Paul Merlin <paul@nosphere.org> wrote:
>>>> 
>>>>> Hi Tibor,
>>>>> 
>>>>> Tibor Mlynarik a écrit :
>>>>>> Hi,
>>>>>> 
>>>>>> I have noticed code in JSONEntityState that seems suspicious for
me.
>>>>>> 
>>>>>> In method JSONEntityState#cloneStateIfGlobalStateLoaded
>>>>>> if parts are swapped.
>>>>>> 
>>>>>> I suppose that by global state is meant copy taken from cache that
is
>>>> shared across UoWs.
>>>>>> And before first entity change own copy for UoW should be cloned.
>>>>>> 
>>>>>> But as it is now, cloning is not done at first change and redundantly
>>>> executed with each entity change.
>>>>>> Also maybe whole cloning could be avoided if cache is not used.
>>>>>> 
>>>>>> Or am I wrong in understanding of purpose of this state clone ?
>>>>> IIRC this is the sole purpose of this state clone.
>>>>> As you pointed out there may be room for improvements here.
>>>>> 
>>>>> Niclas, WDYT?
>>>>> 
>>>>> /Paul
>>>>> 
>>>> 
>>>> 
>>> 
>>> 
>>> --
>>> Niclas Hedhman, Software Developer
>>> http://zest.apache.org - New Energy for Java
>> 
>> 
> 
> 
> -- 
> Niclas Hedhman, Software Developer
> http://zest.apache.org - New Energy for Java


Mime
View raw message