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 :)

>
>>> 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

Reply via email to