Down to 18 failures on 2LC...
Cheers,
--
Galder Zamarreño
Infinispan, Red Hat
On 10 Mar 2017, at 11:38, Galder Zamarreño <galder(a)redhat.com>
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 <rvansa(a)redhat.com> 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-obje...
>
> 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 <rvansa(a)redhat.com> 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(a)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-infinisp...
>>>>>
>>>>> 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)
>>> Hmmmmm, 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
>> EntryWrappingInterceptor/EntryWrappingFactory rather than in each
>> command - in #4564 [2] I wanted to narrow the possible situations to be
>> handled in perform(), and other pieces (e.g. return handlers - see
>> dataReadReturnHandler, that will throw NPE as well in TX RR right now).
>> Note that some commands do the check ATM (e.g. functional commands) - I
>> think that those are leftovers and should be gone.
>>
>> I was about to suggest adding
>>
>> if (!key.equals(key)) return null;
>>
>> to all EWI.visitXXX methods as proper implemenations of equals should do the
>>
>> if (other == this) return true;
>>
>> anyway as a shortcut, so such check shouldn't affect performance.
>> However, this is not as simple with functional commands which should
>> operate on the null entry (on some node), we have to run these, and you
>> can't even ignore the value, you need some entry in there.
>>
>> I hate not finding my socks in the drawer where I definitely put those.
>> This just breaks too many things :-(
>>
>> Btw., it seems that SingleTxContextInvocationContext [1] will fail when
>> the entry is loaded from cache store - this is the place where you could
>> put the entry into invocation context twice. So the use case was
>> somewhat broken before, too.
>>
>>
>> [1]
>>
https://github.com/infinispan/infinispan/blob/master/core/src/main/java/o...
>> [2]
https://github.com/infinispan/infinispan/pull/4564
>>
>>
>>> 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/o...
>>>>>>
>>>>>>> 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-infinisp...
>>>>>>> --
>>>>>>> Galder Zamarreño
>>>>>>> Infinispan, Red Hat
>>>>>>>
>>>>>> --
>>>>>> Radim Vansa <rvansa(a)redhat.com>
>>>>>> JBoss Performance Team
>>>> --
>>>> Radim Vansa <rvansa(a)redhat.com>
>>>> JBoss Performance Team
>>>>
>>
>
>
> --
> Radim Vansa <rvansa(a)redhat.com>
> JBoss Performance Team
>
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev(a)lists.jboss.org
>
https://lists.jboss.org/mailman/listinfo/infinispan-dev
_______________________________________________
infinispan-dev mailing list
infinispan-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/infinispan-dev