Quantcast

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

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Galder Zamarreño
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.

Moreover, if EntryLookup.lookupEntry javadoc (and some implementations) can and do return null. Are you expecting that somehow that method will never return null?

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


_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Galder Zamarreño
Umbrella JIRA: https://issues.jboss.org/browse/ISPN-7475
For this specific issue: https://issues.jboss.org/browse/ISPN-7476

Cheers,
--
Galder Zamarreño
Infinispan, Red Hat

> On 15 Feb 2017, at 11:28, Galder Zamarreño <[hidden email]> 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.
>
> Moreover, if EntryLookup.lookupEntry javadoc (and some implementations) can and do return null. Are you expecting that somehow that method will never return null?
>
> 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
>
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev


_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Radim Vansa
In reply to this post by Galder Zamarreño
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? 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.

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

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 <[hidden email]>
JBoss Performance Team

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Galder Zamarreño

--
Galder Zamarreño
Infinispan, Red Hat

> On 15 Feb 2017, at 12:21, Radim Vansa <[hidden email]> 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 ;)

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

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 <[hidden email]>
> JBoss Performance Team


_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Galder Zamarreño
p.s. I'm yet to verify whether such a misbehaving key produce such NPE in Infinispan... could be the case that this is all caused by my integration efforts in 2LC...

Cheers,
--
Galder Zamarreño
Infinispan, Red Hat

> On 15 Feb 2017, at 18:07, Galder Zamarreño <[hidden email]> wrote:
>
>
> --
> Galder Zamarreño
> Infinispan, Red Hat
>
>> On 15 Feb 2017, at 12:21, Radim Vansa <[hidden email]> 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 ;)
>
>>
>>>
>>> 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?
>
> 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 <[hidden email]>
>> JBoss Performance Team
>
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev


_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Radim Vansa
In reply to this post by Galder Zamarreño
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 <[hidden email]> 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 <[hidden email]>
>> JBoss Performance Team


--
Radim Vansa <[hidden email]>
JBoss Performance Team

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Radim Vansa
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 <[hidden email]> 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.

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 <[hidden email]>
>>> JBoss Performance Team
>


--
Radim Vansa <[hidden email]>
JBoss Performance Team

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

William Burns-3


On Thu, Feb 16, 2017 at 8:51 AM Radim Vansa <[hidden email]> 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 <[hidden email]> 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 <[hidden email]>
>>> JBoss Performance Team
>


--
Radim Vansa <[hidden email]>
JBoss Performance Team

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Radim Vansa
On 02/16/2017 02:53 PM, William Burns wrote:

>
>
> On Thu, Feb 16, 2017 at 8:51 AM Radim Vansa <[hidden email]
> <mailto:[hidden email]>> 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 <[hidden email]
>     <mailto:[hidden email]>> 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 :)

It is, but with @Deprecated (with capital D) in javadoc it did not show
up properly in IntelliJ. I've fixed those [1].

IMO a warning message/exception when an user tries to set up equivalence
other than AnyEquivalence would be even better, silently ignoring that
could lead to obscure problems (when the equivalence definition in the
object and equivalence function differs only in minority of cases).

R.

[1] https://github.com/infinispan/infinispan/pull/4865

>
>     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 <[hidden email] <mailto:[hidden email]>>
>     >>> JBoss Performance Team
>     >
>
>
>     --
>     Radim Vansa <[hidden email] <mailto:[hidden email]>>
>     JBoss Performance Team
>
>     _______________________________________________
>     infinispan-dev mailing list
>     [hidden email] <mailto:[hidden email]>
>     https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
>
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev


--
Radim Vansa <[hidden email]>
JBoss Performance Team

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Loading...