[infinispan-dev] GetKeyValueCommand NPE with CR1 in Hibernate 2LC - ISPN-7029

[William Burns]

On Thu, Feb 16, 2017 at 8:51 AM Radim Vansa <rvansa at 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 <rvansa at 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 <rvansa at redhat.com>
> >>> JBoss Performance Team
> >
>
>
> --
> Radim Vansa <rvansa at redhat.com>
> JBoss Performance Team
>
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/infinispan-dev/attachments/20170216/deea6380/attachment.html