polygene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Niclas Hedhman <nic...@hedhman.org>
Subject Re: JSONEntityState internal clone logic
Date Mon, 27 Jul 2015 15:36:04 GMT
Excellent... I am really happy that you validated the effect in full.
Breeds confidence.

Thanks

On Mon, Jul 27, 2015 at 11:14 PM, Paul Merlin <paul@nosphere.org> wrote:

> Guys,
>
> I reviewed JSONEntityState cloning and Tibor catched an obvious mistake.
>
> The cloneStateIfGlobalStateLoaded() method of JSONEntityState is called
> before any change is made to the entity state.
>
> This method starts with the following check:
>
> if( isStateNotCloned() )
> {
>     return;
> }
> // Do the clone
>
> And the (private) isStateNotCloned() method returns:
> status == EntityStatus.LOADED
>
> IOW, the code says that it will clone state if status is loaded, but it
> does the opposite!
>
> Corresponding JIRA issue: https://issues.apache.org/jira/browse/ZEST-107.
>
>
> Rewriting the cloneStateIfGlobalStateLoaded() method, as Tibor suggested,
> fixes the issue:
>
> if( status != EntityState.LOADED )
> {
>     return;
> }
> // Do the clone
>
> I removed the isStateNotCloned() method BTW as it was not used elsewhere
> and the code is simpler without it.
>
>
> Moreover, I reworded the test case Tibor provided so it fits in
> AbstractEntityStoreTest as this:
>
> @Test
> public void
> givenEntityStoredLoadedChangedWhenUnitOfWorkDiscardsThenDontStoreState()
>     throws UnitOfWorkCompletionException
> {
>     UnitOfWork unitOfWork = module.newUnitOfWork();
>     try
>     {
>         String identity = createEntity( unitOfWork ).identity().get();
>         unitOfWork.complete();
>
>         unitOfWork = module.newUnitOfWork();
>         TestEntity entity = unitOfWork.get( TestEntity.class, identity );
>         assertThat( entity.intValue().get(), is( 42 ) );
>         entity.intValue().set( 23 );
>         unitOfWork.discard();
>
>         unitOfWork = module.newUnitOfWork();
>         entity = unitOfWork.get( TestEntity.class, identity );
>         assertThat( entity.intValue().get(), is( 42 ) ); // Fail here!
>     }
>     finally
>     {
>         unitOfWork.discard();
>     }
> }
>
> This test pass when no CachePool service is assembled.
>
> Without the fix described above, and as soon as a CachePool service is
> used by (visible to) the EntityStore, this test fail: the cached version is
> returned and the discarded change (intValue from 42 to 23) is loaded when
> it shouldn't be.
>
> I then added some AbstractEntityStoreWithCacheTest in core/testsupport to
> reuse all tests from AbstractEntityStoreTest adding a MemoryCachePool to
> the assembly (overridable). And implemented it for all of our EntityStores
> that suppots the Cache SPI (all - [Preferences, SQL]).
>
> So far so good, all SDK tests passes with theses changes.
>
> I did that work in a branch, merged into develop as a single commit, "à
> la" gitflow, so we can revert all this easily if needed and keep the
> complete history.
>
> Any comments?
>
> /Paul
>
> BTW, see https://issues.apache.org/jira/browse/INFRA-8434 for how to
> close github pull requests from asf repositories
>
>
>
> Tibor Mlynarik a écrit :
> > 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
> >
>



-- 
Niclas Hedhman, Software Developer
http://zest.apache.org - New Energy for Java

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message