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

[Galder Zamarreño]

Looks good on 2LC, so I've added it to this PR containing 2LC related fixes:
https://github.com/infinispan/infinispan/pull/4960

Down to 18 failures on 2LC...

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

> On 10 Mar 2017, at 11:38, Galder Zamarreño  wrote:
> 
> Neat! Just tried quickly and seems to work on ISPN test... I'll see if it 
> works in 2LC and if so I'll send a PR. 
> 
> We can then inspect where else this check might be needed.
> 
> Cheers,
> --
> Galder Zamarreño
> Infinispan, Red Hat
> 
>> On 27 Feb 2017, at 14:01, Radim Vansa  wrote:
>> 
>> Hi Galder,
>> 
>> another solution came upon my mind (actually after finding this SO 
>> question [1]): we could check if
>> 
>> (this.key == key || this.key.equals(key))
>> 
>> in SingleKeyNonTxInvocation context instead of just
>> 
>> this.key.equals(key)
>> 
>> and that could do the trick. Plus, such check maybe needed in couple of 
>> other places, haven't tried that.
>> 
>> Seems that HashMap already does so, so the other types of contexts 
>> should work OOTB.
>> 
>> Radim
>> 
>> [1] 
>> http://stackoverflow.com/questions/13521184/equals-returns-false-yet-object-is-found-in-map
>> 
>> On 02/20/2017 05:22 PM, Radim Vansa wrote:
>>> On 02/20/2017 03:52 PM, Galder Zamarreño wrote:
 I've just verified the problem and the NPE can be reproduced with 
 Infinispan alone.
 
 More replies below:
 
 --
 Galder Zamarreño
 Infinispan, Red Hat
 
> On 16 Feb 2017, at 10:44, 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  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)
 Hm, what about not throwing an exception at all?
 
 IOW, e.g. in SingleKeyNonTxInvocationContext.lookupEntry(), if the key is 
 not equals to the one cached, but the cached one is NullCacheEntry, 
 couldn't it return that instead of null? What I'm trying to achieve here 
 is that if the equals is not correctly implemented, the get() returns 
 null, rather than throwing an 

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

[Galder Zamarreño]

Neat! Just tried quickly and seems to work on ISPN test... I'll see if it works 
in 2LC and if so I'll send a PR. 

We can then inspect where else this check might be needed.

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

> On 27 Feb 2017, at 14:01, Radim Vansa  wrote:
> 
> Hi Galder,
> 
> another solution came upon my mind (actually after finding this SO 
> question [1]): we could check if
> 
> (this.key == key || this.key.equals(key))
> 
> in SingleKeyNonTxInvocation context instead of just
> 
> this.key.equals(key)
> 
> and that could do the trick. Plus, such check maybe needed in couple of 
> other places, haven't tried that.
> 
> Seems that HashMap already does so, so the other types of contexts 
> should work OOTB.
> 
> Radim
> 
> [1] 
> http://stackoverflow.com/questions/13521184/equals-returns-false-yet-object-is-found-in-map
> 
> On 02/20/2017 05:22 PM, Radim Vansa wrote:
>> On 02/20/2017 03:52 PM, Galder Zamarreño wrote:
>>> I've just verified the problem and the NPE can be reproduced with 
>>> Infinispan alone.
>>> 
>>> More replies below:
>>> 
>>> --
>>> Galder Zamarreño
>>> Infinispan, Red Hat
>>> 
 On 16 Feb 2017, at 10:44, 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  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)
>>> Hm, what about not throwing an exception at all?
>>> 
>>> IOW, e.g. in SingleKeyNonTxInvocationContext.lookupEntry(), if the key is 
>>> not equals to the one cached, but the cached one is NullCacheEntry, 
>>> couldn't it return that instead of null? What I'm trying to achieve here is 
>>> that if the equals is not correctly implemented, the get() returns null, 
>>> rather than throwing an exception.
>>> 
>>> We'd also need to verify whether other context implementations could deal 
>>> with it.
>>> 
>>> Wouldn't that be better :\ ? I can see the point of throwing an exception 
>>> (other than NPE of course), but it feels a little abrupt... particularly 
>>> since seems like previously we've just returned null in such situations.
>> You're right that Cache.get(x) should ideally return null.
>> 
>> 

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

[Radim Vansa]

Hi Galder,

another solution came upon my mind (actually after finding this SO 
question [1]): we could check if

(this.key == key || this.key.equals(key))

in SingleKeyNonTxInvocation context instead of just

this.key.equals(key)

and that could do the trick. Plus, such check maybe needed in couple of 
other places, haven't tried that.

Seems that HashMap already does so, so the other types of contexts 
should work OOTB.

Radim

[1] 
http://stackoverflow.com/questions/13521184/equals-returns-false-yet-object-is-found-in-map

On 02/20/2017 05:22 PM, Radim Vansa wrote:
> On 02/20/2017 03:52 PM, Galder Zamarreño wrote:
>> I've just verified the problem and the NPE can be reproduced with Infinispan 
>> alone.
>>
>> More replies below:
>>
>> --
>> Galder Zamarreño
>> Infinispan, Red Hat
>>
>>> On 16 Feb 2017, at 10:44, 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  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)
>> Hm, what about not throwing an exception at all?
>>
>> IOW, e.g. in SingleKeyNonTxInvocationContext.lookupEntry(), if the key is 
>> not equals to the one cached, but the cached one is NullCacheEntry, couldn't 
>> it return that instead of null? What I'm trying to achieve here is that if 
>> the equals is not correctly implemented, the get() returns null, rather than 
>> throwing an exception.
>>
>> We'd also need to verify whether other context implementations could deal 
>> with it.
>>
>> Wouldn't that be better :\ ? I can see the point of throwing an exception 
>> (other than NPE of course), but it feels a little abrupt... particularly 
>> since seems like previously we've just returned null in such situations.
> You're right that Cache.get(x) should ideally return null.
>
> InvocationContext.lookupEntry() returning NullCacheEntry and null has a
> different meaning, though:
>
> * NullCacheEntry means that we are supposed to be able to read that
> entry (according to readCH) but we haven't found that in DC. This is
> used for example in CacheLoaderInterceptor, where we skip entries that
> aren't in the context as opposed to recomputing the hash of the key.
> * null means that this entry cannot be read on this node because 

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

[Radim Vansa]

On 02/20/2017 03:52 PM, Galder Zamarreño wrote:
> I've just verified the problem and the NPE can be reproduced with Infinispan 
> alone.
>
> More replies below:
>
> --
> Galder Zamarreño
> Infinispan, Red Hat
>
>> On 16 Feb 2017, at 10:44, 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  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)
> Hm, what about not throwing an exception at all?
>
> IOW, e.g. in SingleKeyNonTxInvocationContext.lookupEntry(), if the key is not 
> equals to the one cached, but the cached one is NullCacheEntry, couldn't it 
> return that instead of null? What I'm trying to achieve here is that if the 
> equals is not correctly implemented, the get() returns null, rather than 
> throwing an exception.
>
> We'd also need to verify whether other context implementations could deal 
> with it.
>
> Wouldn't that be better :\ ? I can see the point of throwing an exception 
> (other than NPE of course), but it feels a little abrupt... particularly 
> since seems like previously we've just returned null in such situations.

You're right that Cache.get(x) should ideally return null.

InvocationContext.lookupEntry() returning NullCacheEntry and null has a 
different meaning, though:

* NullCacheEntry means that we are supposed to be able to read that 
entry (according to readCH) but we haven't found that in DC. This is 
used for example in CacheLoaderInterceptor, where we skip entries that 
aren't in the context as opposed to recomputing the hash of the key.
* null means that this entry cannot be read on this node because the 
segment this key belongs to is not in readCH, and therefore the command 
shouldn't get past distribution interceptor.

I don't mind changing the 'design' as long as it follows some simple 
rules. I don't see how returning NCE for *any* key could be incorporated 
into the rules, and how should that behave on multi-key contexts.

In all but distribution mode, during read the key must be always wrapped 
for read, because there's either no readCH (local) or replicated CH 
(repl/invalidation). But in invalidation/local mode we can't handle that 
in distribution interceptor as this is not present.

I would really prefer to see this handled in 

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

[Galder Zamarreño]

I've just verified the problem and the NPE can be reproduced with Infinispan 
alone.

More replies below:

--
Galder Zamarreño
Infinispan, Red Hat

> On 16 Feb 2017, at 10:44, 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  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)

Hm, what about not throwing an exception at all?

IOW, e.g. in SingleKeyNonTxInvocationContext.lookupEntry(), if the key is not 
equals to the one cached, but the cached one is NullCacheEntry, couldn't it 
return that instead of null? What I'm trying to achieve here is that if the 
equals is not correctly implemented, the get() returns null, rather than 
throwing an exception.

We'd also need to verify whether other context implementations could deal with 
it.

Wouldn't that be better :\ ? I can see the point of throwing an exception 
(other than NPE of course), but it feels a little abrupt... particularly since 
seems like previously we've just returned null in such situations.

Cheers,

> 
> 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 
>>> JBoss Performance Team
> 
> 
> -- 
> Radim Vansa 
> JBoss Performance Team
> 

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

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

[William Burns]

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

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  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 
>>> JBoss Performance Team
>


-- 
Radim Vansa 
JBoss Performance Team

___
infinispan-dev mailing list
infinispan-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/infinispan-dev

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

[Radim Vansa]

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  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 
>> JBoss Performance Team


-- 
Radim Vansa 
JBoss Performance Team

___
infinispan-dev mailing list
infinispan-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/infinispan-dev

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  wrote:
> 
> 
> --
> Galder Zamarreño
> Infinispan, Red Hat
> 
>> On 15 Feb 2017, at 12:21, Radim Vansa  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 
>> 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

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  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 
> JBoss Performance Team


___
infinispan-dev mailing list
infinispan-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/infinispan-dev

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

[Radim Vansa]

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 
JBoss Performance Team

___
infinispan-dev mailing list
infinispan-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/infinispan-dev

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