[infinispan-dev] ISPN-1384 - InboundInvocationHandlerImpl should wait for cache to be started? (not just defined)

[Galder Zamarreño]

On Sep 28, 2011, at 7:56 PM, Dan Berindei wrote:

> On Wed, Sep 28, 2011 at 6:21 PM, Galder Zamarreño <galder at redhat.com> wrote:
>> 
>> On Sep 28, 2011, at 4:06 PM, Galder Zamarreño wrote:
>> 
>>> 
>>> On Sep 27, 2011, at 6:50 PM, Dan Berindei wrote:
>>> 
>>>> On Tue, Sep 27, 2011 at 5:59 PM, Galder Zamarreño <galder at redhat.com> wrote:
>>>>> Hi,
>>>>> 
>>>>> Re: https://issues.jboss.org/browse/ISPN-1384
>>>>> 
>>>>> I've had a look to this and this race condition could, in theory, be resolved by making InboundInvocationHandlerImpl.handle() waiting for cache not only to be defined, but to be started. Otherwise there'll always be a potential race condition like the one showed in the log.
>>>>> 
>>>> 
>>>> Actually I think it would be enough to wait until the cache has
>>>> started joining (StateTransferManager.waitForJoinToStart()), that
>>>> means all the other components have finished starting.
>>> 
>>> Sounds good to me.
>> 
>> Couple of things:
>> 
>> 1. In this particular case, waiting for that would work because this clustered get is a cluster wide one, so there's no need for this node to wait for state transfer to occur. InboundInvocationHandlerImpl.handle could be hacked to check on the cache's StateTransferManager.waitForJoinToStart() but seems a bit hacky.
>> 
> 
> It's already calling StateTransferManager.waitForJoinToComplete(), I
> don't think calling another method would make it hackier...

Ah yeah, I had missed that. That should already solve the problem of this clustered get. I'm closing ISPN-1384.

> 
>> 2. In BaseStateTransferManagerImpl.start it says:
>> 
>> "   // needs to be AFTER the DistributionManager and *after* the cache loader manager (if any) inits and preloads"
>> 
>> CacheLoaderManagerImpl.preload has priority 56, compared to BaseStateTransferManagerImpl.start which has 21, so state transfer could start before preloading has occurred?
>> 
> 
> I missed this, I got the comment from the old StateTransferManager but
> I didn't check the actual priorities.

This is something Pete mentioned last week. We need a better way of finding out the exact order of how things start+stop. Right now you have to manually go and check priorities of each @Start/@Stop mentions. 

This could be kept more centrally by making all @Start/@Stop annotations use a centrally defined set of priorities, i.e. 

in BaseStateTransferManagerImpl

@Start(priority = ComponentOrder.STATE_TRANSFER_START_PRIO)

....

In ComponentOrder.java:

START_STATE_TRANSFER_PRIO = 21


> 
> I'm not sure if the comment is valid though, since the old
> StateTransferManager had priority 55 and it also cleared the data
> container before applying the state from the coordinator. I'm not sure
> how preloading and state transfer are supposed to interact, maybe
> Manik can help clear this up?

I think the priorities are still the same: First preload, and then override what's changed with state transfer. Clearly, it'd be wasteful to throw away in memory contents if they were preloaded.

> 
>> 
>>> 
>>>> 
>>>> With the asymmetric cache support in place I think we shouldn't have
>>>> to wait at all, since we'll send the join request only after all the
>>>> components have been started. We could either apply the commands (if
>>>> we implement the non-blocking state transfer option) or reject them
>>>> and tell the originator to retry after the state transfer is done (if
>>>> we keep the blocking state transfer, since the other nodes shouldn't
>>>> be sending commands to us anyway).
>>>> 
>>>>> In this particular case, this is clustered get command being received from a clustered cache loader, which is arriving in the cache before this is started (and the cache loader has been created, hence the NPE).
>>>>> 
>>>>> Another question, is there any reason why CacheLoader is not a named cache component which can be initalised with a corresponding factory and to which other components can be injected (i.e. marshaller, cache...etc)?
>>>>> 
>>>>> In this particular case, this would also resolve the issue because ClusterCacheLoader.start() does nothing, so all the interceptor needs is a proper instance of ClusterCacheLoader available. The factory makes these available bejore inject.
>>>>> 
>>>>> Thoughts?
>>>>> 
>>>>> Cheers,
>>>>> 
>>>>> p.s. Dan, I am aware of https://issues.jboss.org/browse/ISPN-1324, maybe you're solving this indirectly with the work for that JIRA?
>>>> 
>>>> With asymmetric caches in place I don't think we'll need/want
>>>> ISPN-1324 any more.
>>> 
>>> +1
>>> 
>>>> 
>>>> Cheers
>>>> Dan
>>>> 
>>>> 
>>>>> --
>>>>> Galder Zamarreño
>>>>> Sr. Software Engineer
>>>>> Infinispan, JBoss Cache
>>>>> 
>>>>> 
>>>>> _______________________________________________
>>>>> infinispan-dev mailing list
>>>>> infinispan-dev at lists.jboss.org
>>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>>> 
>>>> 
>>>> _______________________________________________
>>>> infinispan-dev mailing list
>>>> infinispan-dev at lists.jboss.org
>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>> 
>>> --
>>> Galder Zamarreño
>>> Sr. Software Engineer
>>> Infinispan, JBoss Cache
>>> 
>>> 
>>> _______________________________________________
>>> infinispan-dev mailing list
>>> infinispan-dev at lists.jboss.org
>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>> 
>> --
>> Galder Zamarreño
>> Sr. Software Engineer
>> Infinispan, JBoss Cache
>> 
>> 
>> _______________________________________________
>> infinispan-dev mailing list
>> infinispan-dev at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>> 

--
Galder Zamarreño
Sr. Software Engineer
Infinispan, JBoss Cache