[infinispan-dev] ISPN-2314 fix looks fishy

[Galder Zamarreño]

On Sep 19, 2012, at 1:02 PM, Mircea Markus <mircea.markus at jboss.com> wrote:

> 
> On 19 Sep 2012, at 12:57, Manik Surtani wrote:
> 
>>> 
>>> Hi guys,
>>> 
>>> Having briefly investigated https://issues.jboss.org/browse/ISPN-2314, I consider the fix (https://github.com/infinispan/infinispan/pull/1318) to be rather fishy.
>>> 
>>> Sure it works, but it masks the fact that the order in which things are started has changed (This IMO seems to be the result of http://issues.jboss.org/browse/ISPN-2256). 
>>> 
>>> Not only that, but the fact that you have to modify both the component registry and the configuration makes me think that there're situations where the configuration changes are applied, and others where it's not (hence why you have to change the component registry).
>>> 
>>> So, can we please stop and try to understand what really is happening here? I was expecting Mircea to have a look into a fix for this since it looks related to cache start order.
>>> 
>>> Personally, I don't really see why the lifecycle manager has to mess up with the component registry. Changes to the configuration were 'enough' (until 2256) and I don't see why that should change.
>> 
>> It appears that the cache is already constructed when the cache starting callback is issued.  This means any changes to the configuration at this point won't have any effect.  But changing the configuration is still necessary to allow for restarts.
> 
> The cacheStartingCallback was being invoked from two places:
> - when the cache was first created from InternalCacheFactory
> - on subsequent  re-starts from ComponentRegistry.start() 
> 
> As the same call happened at different stages of initialisation I've only kept the 2nd call for consistency. 

At the time of ISPN-2232, there was only a place where from componentRegistry.notifyCacheStarting(configuration) was being called, and that was from from InternalCacheFactory.

The call from ComponentRegistry was added as part of ISPN-2254 (https://github.com/infinispan/infinispan/commit/6ffef479ec40dd25b7195dea0e7800a2837dfb97).

TBH, it's unclear when exactly is cacheStarting() is called. IOW, if I make changes to the configuration passed, will they be applied to the cache? At the time of ISPN-2232, yes. Now, no.

For me, the best way for Lifecycle callbacks to make their changes should be via the configuration, so that callback should happen (both on start and restart), in such way, that it allows the configuration to be (re)built, (re)applied to the cache. IOW, lifecycle callbacks shouldn't need to implement two mechanisms to make the changes (one vi config, and the other messing with the component registry)

> 
> Cheers,
> -- 
> Mircea Markus
> Infinispan lead (www.infinispan.org)
> 
> 
> 
> 
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/infinispan-dev


--
Galder Zamarreño
galder at redhat.com
twitter.com/galderz

Project Lead, Escalante
http://escalante.io

Engineer, Infinispan
http://infinispan.org