On Thu, Feb 16, 2017 at 8:51 AM Radim Vansa <rva...@redhat.com> wrote:
> On 02/16/2017 10:44 AM, Radim Vansa wrote: > > On 02/15/2017 06:07 PM, Galder Zamarreño wrote: > >> -- > >> Galder Zamarreño > >> Infinispan, Red Hat > >> > >>> On 15 Feb 2017, at 12:21, Radim Vansa <rva...@redhat.com> wrote: > >>> > >>> On 02/15/2017 11:28 AM, Galder Zamarreño wrote: > >>>> Hey Radim, > >>>> > >>>> Your changes in ISPN-7029 are causing issues with Hibernate 2LC. > >>>> > >>>> In particular [2]. Name.equals() always returns false, so it'll never > be found in the context. So entry is null. > >>> That's obviously a bug in the 2LC testsuite, isn't it? > >> LOL, is it? You added the class and the javadoc clearly states that > this entity defines equals incorrectly. You must have added it for a reason? > >> > >> > https://github.com/hibernate/hibernate-orm/blob/master/hibernate-infinispan/src/test/java/org/hibernate/test/cache/infinispan/functional/entities/Name.java > >> > >> In any case, an object with an incorrect equals impl should not result > in an NPE within Infinispan :| > >> > >>> Object used as @EmbeddedId needs to have the equals correctly defined. > How else would you compare ids? I wonder how could that work in the past. > >> ROFL... it's your baby so you tell me ;) > > Okay, okay - I haven't checked the javadoc, so I just assumed that it's > > an oversight :) > > The reason it worked before was that both DC and EntryLookup used > keyEquivalence for all the comparisons, and 2LC was providing > TypeEquivalence there. In 9.0 (master) the keyEquivalence was dropped. > For some reason DataContainerConfigurationBuilder.keyEquivalence is not > marked as deprecated, I'll file a PR for that. > The entire DataContainerConfiguration and Builder classes are deprecated as well as Equivalence :) > > R. > > > > >>>> Moreover, if EntryLookup.lookupEntry javadoc (and some > implementations) can and do return null. Are you expecting that somehow > that method will never return null? > >>> With ISPN-7029 in, the entry should be wrapped in the context after > EntryWrappingInterceptor if the key is in readCH, otherwise it should be > null. In case that xDistributionInterceptor finds out that it needs to work > on that value despite not being able to read it (e.g. in case that it's in > writeCH during unfinished rebalance), it should wrap > NullCacheEntry.getInstance() using EntryFactory. wrapExternalEntry. More > info about the logic is in o.i.c.EntryFactory javadoc [3]. > >> Not sure I understand what you're trying to imply above... so, is > lookupEntry() allowed to return null or not? > > It is allowed to return null, but: > > > > 1. If the node is an owner according to readCH, the entry must be > > wrapped into context in EWI (possibly as NullCacheEntry). > > 2. The code can reach the GKVC.perform() iff this node is an owner of > > given key. > > > > The problem here is that I've assumed that if the entry was wrapped, it > > can be fetched. With incorrectly defined equals, as we see here, this > > does not hold. So we can > > > > a) check if the entry is null and throw more explanatory exception - > > more code in perform() > > b) do the lookup after wrapping and throw there - unnecessary map lookup > > for such annoying problem > > c) ostrich approach > > > > I think that b) in assert could do, otherwise I'd suggest c) > > > > Radim > > > >> To be more specific, SingleKeyNonTxInvocationContext.lookupEntry() can > return null, so GetKeyValueCommand should be able to handle it? Or should > SingleKeyNonTxInvocationContext.lookupEntry return > NullCacheEntry.getInstance instead of null? > >> > >> To provide more specifics, SingleKeyNonTxInvocationContext has > NullCacheEntry.getInstance in cacheEntry variable when it's returning null. > Should it maybe return NullCacheEntry.getInstance instead of null from > lookupEntry() ? > >> > >> Cheers, > >> > >>> Radim > >>> > >>> [3] > https://github.com/infinispan/infinispan/blob/master/core/src/main/java/org/infinispan/container/EntryFactory.java > >>> > >>>> I'll create a JIRA to track all issues arising from Hibernate 2LC in > a minute, but wanted to get your thoughts firts. > >>>> > >>>> Cheers, > >>>> > >>>> [1] https://issues.jboss.org/browse/ISPN-7029 > >>>> [2] > https://github.com/hibernate/hibernate-orm/blob/master/hibernate-infinispan/src/test/java/org/hibernate/test/cache/infinispan/CacheKeysFactoryTest.java#L58 > >>>> -- > >>>> Galder Zamarreño > >>>> Infinispan, Red Hat > >>>> > >>> -- > >>> Radim Vansa <rva...@redhat.com> > >>> JBoss Performance Team > > > > > -- > Radim Vansa <rva...@redhat.com> > JBoss Performance Team > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev@lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev
_______________________________________________ infinispan-dev mailing list infinispan-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev