The goal being to resolve ISPN-4561, I was thinking to expose a very simple reference counter in the AdvancedCache API. As you know the Query module - which triggers on indexed caches - can use the Infinispan Lucene Directory to store its indexes in a (different) Cache. When the CacheManager is stopped, if the index storage caches are stopped first, then the indexed cache is stopped, this might need to flush/close some pending state on the index and this results in an illegal operation as the storate is shut down already. We could either implement a complex dependency graph, or add a method like: boolean incRef(); on AdvancedCache. when the Cache#close() method is invoked, this will do an internal decrement, and only when hitting zero it will really close the cache. A CacheManager shutdown will loop through all caches, and invoke close() on all of them; the close() method should return something so that the CacheManager shutdown loop understand if it really did close all caches or if not, in which case it will loop again through all caches, and loops until all cache instances are really closed. The return type of "close()" doesn't necessarily need to be exposed on public API, it could be an internal only variant.

Could we do this? --Sanne _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On 15 August 2014 14:55, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: > It looks to me like you actually want a partial order between caches on > shutdown, so why not declare an explicit dependency (e.g. > manager.stopOrder(before, after)? We could even throw an exception if the > user tries to stop a cache manually in the wrong order (e.g. > TestingUtil.killCacheManagers). Because that's much more complex to implement? incRef() seems trivial, effective and can be used by other components for different patterns. > Alternatively, we could add an event CacheManagerStopEvent(pre=true) at the > cache manager level that is invoked before any cache is stopped, and you > could close all the indexes in that listener. The event could even be at the > cache level, if it would make things easier. I like that more than defining explicit dependency links and it would probably be good enough for this specific problem but I feel like it doesn't solve similar problems with a more complex dependency sequence of services. Counters are effectively providing the same semantics, just that you can use the pre-close pattern nesting it "count times". Also having ref-counting available makes it easier for users to implement independent components - with an independent lifecycle - which might share the same cache. -- Sanne > > Cheers > Dan > > > > On Fri, Aug 15, 2014 at 3:29 PM, Sanne Grinovero <sanne(a)infinispan.org&gt; > wrote: >> >> The goal being to resolve ISPN-4561, I was thinking to expose a very >> simple reference counter in the AdvancedCache API. >> >> As you know the Query module - which triggers on indexed caches - can >> use the Infinispan Lucene Directory to store its indexes in a >> (different) Cache. >> When the CacheManager is stopped, if the index storage caches are >> stopped first, then the indexed cache is stopped, this might need to >> flush/close some pending state on the index and this results in an >> illegal operation as the storate is shut down already. >> >> We could either implement a complex dependency graph, or add a method >> like: >> >> >> boolean incRef(); >> >> on AdvancedCache. >> >> when the Cache#close() method is invoked, this will do an internal >> decrement, and only when hitting zero it will really close the cache. >> >> A CacheManager shutdown will loop through all caches, and invoke >> close() on all of them; the close() method should return something so >> that the CacheManager shutdown loop understand if it really did close >> all caches or if not, in which case it will loop again through all >> caches, and loops until all cache instances are really closed. >> The return type of "close()" doesn't necessarily need to be exposed on >> public API, it could be an internal only variant. >> >> >> Could we do this? >> >> --Sanne >> _______________________________________________ >> 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 _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

Ales, I don't think the implementation matters that much, I was only concerned about the API. BTW, where could I find some documentation on MSC? Sanne, I missed something in your initial email: you mention a Cache.close() method, did you mean Cache.stop(), or did you mean to add a new close() method?

Cache doesn't actually define a stop() method, it inherits the stop() method from the Lifecycle interface. So changing its semantics only for caches would be hacky. Adding a different close() method would be better, but it still wouldn't be my first choice... On Sat, Aug 16, 2014 at 12:40 AM, Ales Justin <ales.justin(a)gmail.com&gt; wrote: > > What about if you add an SPI for this? > > e.g. this could be nicely implemented on top of WildFly's MSC > > And by default you would keep this simple incRef, > or some similar simple state machine we used in Microcontainer. > > -Ales > > On 15 Aug 2014, at 16:26, Sanne Grinovero <sanne(a)infinispan.org&gt; wrote: > > > On 15 August 2014 14:55, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: > >> It looks to me like you actually want a partial order between caches on > >> shutdown, so why not declare an explicit dependency (e.g. > >> manager.stopOrder(before, after)? We could even throw an exception if > >> the > >> user tries to stop a cache manually in the wrong order (e.g. > >> TestingUtil.killCacheManagers). > > > > Because that's much more complex to implement? > > incRef() seems trivial, effective and can be used by other components > > for different patterns. Implementing proper dependencies doesn't need to be difficult either, all we need is to keep a list of dependants in the cache and prune the stopped caches from it before doing the check.

incRef might be easier to implement, but instead it seems harder to explain to a user in the Javadoc.

> >> Alternatively, we could add an event CacheManagerStopEvent(pre=true) at > >> the > >> cache manager level that is invoked before any cache is stopped, and > >> you > >> could close all the indexes in that listener. The event could even be > >> at the > >> cache level, if it would make things easier. > > > > I like that more than defining explicit dependency links and it would > > probably be good enough for this specific problem > > but I feel like it doesn't solve similar problems with a more complex > > dependency sequence of services. > > Counters are effectively providing the same semantics, just that you > > can use the pre-close pattern nesting it "count times". > > > > Also having ref-counting available makes it easier for users to > > implement independent components - with an independent lifecycle - > > which might share the same cache. By independent components do you mean global components? That wouldn't work, since we only start stopping global components after we have stopped all the caches - regardless of the order in which we stop caches.

A global pre-stop event, instead, would allow global components to do stuff before any of the caches is stopped.

> > > > -- Sanne > > > >> > >> Cheers > >> Dan > >> > >> > >> > >> On Fri, Aug 15, 2014 at 3:29 PM, Sanne Grinovero <sanne(a)infinispan.org&gt; > >> wrote: > >>> > >>> The goal being to resolve ISPN-4561, I was thinking to expose a very > >>> simple reference counter in the AdvancedCache API. > >>> > >>> As you know the Query module - which triggers on indexed caches - can > >>> use the Infinispan Lucene Directory to store its indexes in a > >>> (different) Cache. > >>> When the CacheManager is stopped, if the index storage caches are > >>> stopped first, then the indexed cache is stopped, this might need to > >>> flush/close some pending state on the index and this results in an > >>> illegal operation as the storate is shut down already. > >>> > >>> We could either implement a complex dependency graph, or add a method > >>> like: > >>> > >>> > >>> boolean incRef(); > >>> > >>> on AdvancedCache. > >>> > >>> when the Cache#close() method is invoked, this will do an internal > >>> decrement, and only when hitting zero it will really close the cache. > >>> > >>> A CacheManager shutdown will loop through all caches, and invoke > >>> close() on all of them; the close() method should return something so > >>> that the CacheManager shutdown loop understand if it really did close > >>> all caches or if not, in which case it will loop again through all > >>> caches, and loops until all cache instances are really closed. > >>> The return type of "close()" doesn't necessarily need to be exposed on > >>> public API, it could be an internal only variant. > >>> > >>> > >>> Could we do this? > >>> > >>> --Sanne > >>> _______________________________________________ > >>> 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 > > _______________________________________________ > > 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 _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On Mon, Aug 18, 2014 at 1:56 PM, Sanne Grinovero <sanne(a)infinispan.org&gt; wrote: > > On 18 August 2014 11:33, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: > > Ales, I don't think the implementation matters that much, I was only > > concerned about the API. BTW, where could I find some documentation on > > MSC? > > > > Sanne, I missed something in your initial email: you mention a > > Cache.close() > > method, did you mean Cache.stop(), or did you mean to add a new close() > > method? > > I meant stop(), sorry. > > > > > Cache doesn't actually define a stop() method, it inherits the stop() > > method > > from the Lifecycle interface. So changing its semantics only for caches > > would be hacky. Adding a different close() method would be better, but > > it > > still wouldn't be my first choice... > > > > > > On Sat, Aug 16, 2014 at 12:40 AM, Ales Justin <ales.justin(a)gmail.com&gt; > > wrote: > >> > >> What about if you add an SPI for this? > >> > >> e.g. this could be nicely implemented on top of WildFly's MSC > >> > >> And by default you would keep this simple incRef, > >> or some similar simple state machine we used in Microcontainer. > >> > >> -Ales > >> > >> On 15 Aug 2014, at 16:26, Sanne Grinovero <sanne(a)infinispan.org&gt; wrote: > >> > >> > On 15 August 2014 14:55, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: > >> >> It looks to me like you actually want a partial order between caches > >> >> on > >> >> shutdown, so why not declare an explicit dependency (e.g. > >> >> manager.stopOrder(before, after)? We could even throw an exception > >> >> if > >> >> the > >> >> user tries to stop a cache manually in the wrong order (e.g. > >> >> TestingUtil.killCacheManagers). > >> > > >> > Because that's much more complex to implement? > >> > incRef() seems trivial, effective and can be used by other components > >> > for different patterns. > > > > > > Implementing proper dependencies doesn't need to be difficult either, > > all we > > need is to keep a list of dependants in the cache and prune the stopped > > caches from it before doing the check. > > I expect you or your team to do it, so your choice :-) > I would also be careful in how you decide to spend a day(week?) vs 1h > to provide a feature which is essentially the same stuff for the user. > And if you go for dependency graphs, prepare to do it transactionally > and concurrently.. I don't see why we would need transactions for dependency graphs any more than we would need them for incRef. > > > > incRef might be easier to implement, but instead it seems harder to > > explain > > to a user in the Javadoc. > > I didn't invent incRef myself, it's common in several other projects > (Lucene for one), > so I expect it to be a commonly understood pattern. > > Also I suggested to add it only on AdvancedCache, as I agree it's > "advanced" users only. AdvancedCache is still public API, so it still needs to be documented. I'm not sure Lucene is a good model here, I looked at Lucene's IndexReader documentation [1] and it doesn't look encouraging: the close javadoc says it "Closes files associated with this index", while the incRef javadoc says "Note that close() simply calls decRef()". I also didn't find any mention of what the initial reference count is. To be clear, I don't have anything against reference counting in general. But I don't think overloading the Lifecycle.stop() method to have a totally different behaviour in Cache is a good idea. [1] http://lucene.apache.org/core/4_0_0/core/org/apache/lucene/index/IndexRea... > > > >> >> Alternatively, we could add an event CacheManagerStopEvent(pre=true) > >> >> at > >> >> the > >> >> cache manager level that is invoked before any cache is stopped, and > >> >> you > >> >> could close all the indexes in that listener. The event could even > >> >> be > >> >> at the > >> >> cache level, if it would make things easier. > >> > > >> > I like that more than defining explicit dependency links and it would > >> > probably be good enough for this specific problem > >> > but I feel like it doesn't solve similar problems with a more complex > >> > dependency sequence of services. > >> > Counters are effectively providing the same semantics, just that you > >> > can use the pre-close pattern nesting it "count times". > >> > > >> > Also having ref-counting available makes it easier for users to > >> > implement independent components - with an independent lifecycle - > >> > which might share the same cache. > > > > > > By independent components do you mean global components? That wouldn't > > work, > > since we only start stopping global components after we have stopped all > > the > > caches - regardless of the order in which we stop caches. > > I didn't meant to add this stopping feature to components, but that > many other components might need an entangled sequence of shutdown of > Caches. Ok, fair enough. > > > > > A global pre-stop event, instead, would allow global components to do > > stuff > > before any of the caches is stopped. > > I haven't seen any need for such a thing so far. Your call, but I > don't think we are in the business of service lifecycle management and > dependency injection frameworks. I think we are in that business, whether we like it or not. > > Alesj is right: at best we should make this an SPI, provide a trivial > implementation and leave the details to be handled by those who > thought about it properly; just that the trivial counter is good > enough for my needs. How would that SPI look? And how would someone be able to provide a better implementation than our "trivial" implementation? > > > Sanne > > > > >> > > >> > -- Sanne > >> > > >> >> > >> >> Cheers > >> >> Dan > >> >> > >> >> > >> >> > >> >> On Fri, Aug 15, 2014 at 3:29 PM, Sanne Grinovero > >> >> <sanne(a)infinispan.org&gt; > >> >> wrote: > >> >>> > >> >>> The goal being to resolve ISPN-4561, I was thinking to expose a > >> >>> very > >> >>> simple reference counter in the AdvancedCache API. > >> >>> > >> >>> As you know the Query module - which triggers on indexed caches - > >> >>> can > >> >>> use the Infinispan Lucene Directory to store its indexes in a > >> >>> (different) Cache. > >> >>> When the CacheManager is stopped, if the index storage caches are > >> >>> stopped first, then the indexed cache is stopped, this might need > >> >>> to > >> >>> flush/close some pending state on the index and this results in an > >> >>> illegal operation as the storate is shut down already. > >> >>> > >> >>> We could either implement a complex dependency graph, or add a > >> >>> method > >> >>> like: > >> >>> > >> >>> > >> >>> boolean incRef(); > >> >>> > >> >>> on AdvancedCache. > >> >>> > >> >>> when the Cache#close() method is invoked, this will do an internal > >> >>> decrement, and only when hitting zero it will really close the > >> >>> cache. > >> >>> > >> >>> A CacheManager shutdown will loop through all caches, and invoke > >> >>> close() on all of them; the close() method should return something > >> >>> so > >> >>> that the CacheManager shutdown loop understand if it really did > >> >>> close > >> >>> all caches or if not, in which case it will loop again through all > >> >>> caches, and loops until all cache instances are really closed. > >> >>> The return type of "close()" doesn't necessarily need to be exposed > >> >>> on > >> >>> public API, it could be an internal only variant. > >> >>> > >> >>> > >> >>> Could we do this? > >> >>> > >> >>> --Sanne > >> >>> _______________________________________________ > >> >>> 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 > >> > _______________________________________________ > >> > 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 > > > > > > > > _______________________________________________ > > 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 _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

Ales, I don't think the implementation matters that much, I was only concerned about the API. BTW, where could I find some documentation on MSC? Perhaps check this? https://docs.jboss.org/author/display/MSC/Home -Ales _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

It looks to me like you actually want a partial order between caches on shutdown, so why not declare an explicit dependency (e.g. manager.stopOrder(before, after)? We could even throw an exception if the user tries to stop a cache manually in the wrong order (e.g. TestingUtil.killCacheManagers). Alternatively, we could add an event CacheManagerStopEvent(pre=true) at the cache manager level that is invoked before any cache is stopped, and you could close all the indexes in that listener. The event could even be at the cache level, if it would make things easier.

Cheers Dan On Fri, Aug 15, 2014 at 3:29 PM, Sanne Grinovero <sanne(a)infinispan.org&gt; wrote: The goal being to resolve ISPN-4561, I was thinking to expose a very simple reference counter in the AdvancedCache API. As you know the Query module - which triggers on indexed caches - can use the Infinispan Lucene Directory to store its indexes in a (different) Cache. When the CacheManager is stopped, if the index storage caches are stopped first, then the indexed cache is stopped, this might need to flush/close some pending state on the index and this results in an illegal operation as the storate is shut down already. We could either implement a complex dependency graph, or add a method like: boolean incRef(); on AdvancedCache. when the Cache#close() method is invoked, this will do an internal decrement, and only when hitting zero it will really close the cache. A CacheManager shutdown will loop through all caches, and invoke close() on all of them; the close() method should return something so that the CacheManager shutdown loop understand if it really did close all caches or if not, in which case it will loop again through all caches, and loops until all cache instances are really closed. The return type of "close()" doesn't necessarily need to be exposed on public API, it could be an internal only variant. Could we do this? --Sanne _______________________________________________ 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

On Mon, Aug 25, 2014 at 10:26 AM, Galder Zamarreño <galder(a)redhat.com&gt; wrote: > > > On 15 Aug 2014, at 15:55, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: > > > It looks to me like you actually want a partial order between caches on > > shutdown, so why not declare an explicit dependency (e.g. > > manager.stopOrder(before, after)? We could even throw an exception if the > > user tries to stop a cache manually in the wrong order (e.g. > > TestingUtil.killCacheManagers). > > > > Alternatively, we could add an event CacheManagerStopEvent(pre=true) at > > the cache manager level that is invoked before any cache is stopped, and you > > could close all the indexes in that listener. The event could even be at the > > cache level, if it would make things easier.

> > Not sure you need the listener event since we already have lifecycle event > callbacks for external modules. > > IOW, couldn’t you do this cache stop ordering with an implementation of > org.infinispan.lifecycle.ModuleLifecycle? On cacheStarting, you could maybe > track each started cache and give it a priority, and then on > cacheManagerStopping use that priority to close caches. Note: I’ve not > tested this and I don’t know if the callbacks happen at the right time to > allow this. Just thinking out loud.

> Unfortunately ModuleLifecycle.cacheManagerStopping is only called _after_ all the caches have been stopped.

> > Cheers, > > > > > Cheers > > Dan > > > > > > > > On Fri, Aug 15, 2014 at 3:29 PM, Sanne Grinovero <sanne(a)infinispan.org&gt; > > wrote: > > The goal being to resolve ISPN-4561, I was thinking to expose a very > > simple reference counter in the AdvancedCache API. > > > > As you know the Query module - which triggers on indexed caches - can > > use the Infinispan Lucene Directory to store its indexes in a > > (different) Cache. > > When the CacheManager is stopped, if the index storage caches are > > stopped first, then the indexed cache is stopped, this might need to > > flush/close some pending state on the index and this results in an > > illegal operation as the storate is shut down already. > > > > We could either implement a complex dependency graph, or add a method > > like: > > > > > > boolean incRef(); > > > > on AdvancedCache. > > > > when the Cache#close() method is invoked, this will do an internal > > decrement, and only when hitting zero it will really close the cache.

> > > > A CacheManager shutdown will loop through all caches, and invoke > > close() on all of them; the close() method should return something so > > that the CacheManager shutdown loop understand if it really did close > > all caches or if not, in which case it will loop again through all > > caches, and loops until all cache instances are really closed. > > The return type of "close()" doesn't necessarily need to be exposed on > > public API, it could be an internal only variant. > > > > Could we do this? > > > > --Sanne > > _______________________________________________ > > 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 > > > -- > Galder Zamarreño > galder(a)redhat.com > twitter.com/galderz > > > _______________________________________________ > 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

On 26 August 2014 18:38, William Burns <mudokonman(a)gmail.com&gt; wrote: > On Mon, Aug 25, 2014 at 3:46 AM, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: >> >> >> >> On Mon, Aug 25, 2014 at 10:26 AM, Galder Zamarreño <galder(a)redhat.com&gt; >> wrote: >>> >>> >>> On 15 Aug 2014, at 15:55, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: >>> >>> > It looks to me like you actually want a partial order between caches on >>> > shutdown, so why not declare an explicit dependency (e.g. >>> > manager.stopOrder(before, after)? We could even throw an exception if the >>> > user tries to stop a cache manually in the wrong order (e.g. >>> > TestingUtil.killCacheManagers). >>> > >>> > Alternatively, we could add an event CacheManagerStopEvent(pre=true) at >>> > the cache manager level that is invoked before any cache is stopped, and you >>> > could close all the indexes in that listener. The event could even be at the >>> > cache level, if it would make things easier. > > I think something like this would be the simplest for now especially, > how this is done though we can still decide. > >>> >>> Not sure you need the listener event since we already have lifecycle event >>> callbacks for external modules. >>> >>> IOW, couldn’t you do this cache stop ordering with an implementation of >>> org.infinispan.lifecycle.ModuleLifecycle? On cacheStarting, you could maybe >>> track each started cache and give it a priority, and then on >>> cacheManagerStopping use that priority to close caches. Note: I’ve not >>> tested this and I don’t know if the callbacks happen at the right time to >>> allow this. Just thinking out loud. > > +1 this is a nice use of what is already in place. The only issue I > see here is that there is no ordering of the lifecycle callbacks if > you had more than 1 callback, which could cause issues if users wanted > to reference certain caches. > >>> >> >> Unfortunately ModuleLifecycle.cacheManagerStopping is only called _after_ >> all the caches have been stopped. > > This seems like a bug, not very nice for ordering of callback methods. > >> >> >>> >>> Cheers, >>> >>> > >>> > Cheers >>> > Dan >>> > >>> > >>> > >>> > On Fri, Aug 15, 2014 at 3:29 PM, Sanne Grinovero <sanne(a)infinispan.org&gt; >>> > wrote: >>> > The goal being to resolve ISPN-4561, I was thinking to expose a very >>> > simple reference counter in the AdvancedCache API. >>> > >>> > As you know the Query module - which triggers on indexed caches - can >>> > use the Infinispan Lucene Directory to store its indexes in a >>> > (different) Cache. >>> > When the CacheManager is stopped, if the index storage caches are >>> > stopped first, then the indexed cache is stopped, this might need to >>> > flush/close some pending state on the index and this results in an >>> > illegal operation as the storate is shut down already. >>> > >>> > We could either implement a complex dependency graph, or add a method >>> > like: >>> > >>> > >>> > boolean incRef(); >>> > >>> > on AdvancedCache. >>> > >>> > when the Cache#close() method is invoked, this will do an internal >>> > decrement, and only when hitting zero it will really close the cache. > > Unfortunately this won't work except in a simple dependency case (you > depend on a cache, but no cache can depend on you). > > Say you have 3 caches (C1, C2, C3). > > The case is C2 depends on C1 and C3 depends on C2. In this case both > C1 and C2 would have a ref count value of 1 and C3 would have 0. This > would allow for C1 and C2 to both be eligible to be closed during the > same iteration. Yea people could use it the wrong way :-D

But you can increment in different patterns than what you described to model a full graph: the important point is to allow users to define an order in *some* way. > I think if we started doing dependencies we would really need to have > some sort of graph to have anything more than the simple case. > > Do we know of other use cases where we may want a dependency graph > explicitly? It seems what you want is solvable with what is in place, > it just has a bug :( True, for my case a two-phases would be good enough *generally speaking* as we don't expect people to index stuff in a Cache which is also used to store an index for a different Cache, but that's a "legal" configuration. Applying Muprhy's law, that means someone will try it out and I'd rather be safe about that. It just so happens that the counter proposal is both trivial and also can handle a quite long ordering chain. I don't understand how it's solvable "with what's in place", could you elaborate?

-- Sanne > >>> > >>> > A CacheManager shutdown will loop through all caches, and invoke >>> > close() on all of them; the close() method should return something so >>> > that the CacheManager shutdown loop understand if it really did close >>> > all caches or if not, in which case it will loop again through all >>> > caches, and loops until all cache instances are really closed. >>> > The return type of "close()" doesn't necessarily need to be exposed on >>> > public API, it could be an internal only variant. >>> > >>> > Could we do this? >>> > >>> > --Sanne _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On 26 August 2014 19:38, William Burns <mudokonman(a)gmail.com&gt; wrote: > On Tue, Aug 26, 2014 at 2:23 PM, Sanne Grinovero <sanne(a)infinispan.org&gt; wrote: >> On 26 August 2014 18:38, William Burns <mudokonman(a)gmail.com&gt; wrote: >>> On Mon, Aug 25, 2014 at 3:46 AM, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: >>>> >>>> >>>> >>>> On Mon, Aug 25, 2014 at 10:26 AM, Galder Zamarreño <galder(a)redhat.com&gt; >>>> wrote: >>>>> >>>>> >>>>> On 15 Aug 2014, at 15:55, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: >>>>> >>>>> > It looks to me like you actually want a partial order between caches on >>>>> > shutdown, so why not declare an explicit dependency (e.g. >>>>> > manager.stopOrder(before, after)? We could even throw an exception if the >>>>> > user tries to stop a cache manually in the wrong order (e.g. >>>>> > TestingUtil.killCacheManagers). >>>>> > >>>>> > Alternatively, we could add an event CacheManagerStopEvent(pre=true) at >>>>> > the cache manager level that is invoked before any cache is stopped, and you >>>>> > could close all the indexes in that listener. The event could even be at the >>>>> > cache level, if it would make things easier. >>> >>> I think something like this would be the simplest for now especially, >>> how this is done though we can still decide. >>> >>>>> >>>>> Not sure you need the listener event since we already have lifecycle event >>>>> callbacks for external modules. >>>>> >>>>> IOW, couldn’t you do this cache stop ordering with an implementation of >>>>> org.infinispan.lifecycle.ModuleLifecycle? On cacheStarting, you could maybe >>>>> track each started cache and give it a priority, and then on >>>>> cacheManagerStopping use that priority to close caches. Note: I’ve not >>>>> tested this and I don’t know if the callbacks happen at the right time to >>>>> allow this. Just thinking out loud. >>> >>> +1 this is a nice use of what is already in place. The only issue I >>> see here is that there is no ordering of the lifecycle callbacks if >>> you had more than 1 callback, which could cause issues if users wanted >>> to reference certain caches. >>> >>>>> >>>> >>>> Unfortunately ModuleLifecycle.cacheManagerStopping is only called _after_ >>>> all the caches have been stopped. >>> >>> This seems like a bug, not very nice for ordering of callback methods. >>> >>>> >>>> >>>>> >>>>> Cheers, >>>>> >>>>> > >>>>> > Cheers >>>>> > Dan >>>>> > >>>>> > >>>>> > >>>>> > On Fri, Aug 15, 2014 at 3:29 PM, Sanne Grinovero <sanne(a)infinispan.org&gt; >>>>> > wrote: >>>>> > The goal being to resolve ISPN-4561, I was thinking to expose a very >>>>> > simple reference counter in the AdvancedCache API. >>>>> > >>>>> > As you know the Query module - which triggers on indexed caches - can >>>>> > use the Infinispan Lucene Directory to store its indexes in a >>>>> > (different) Cache. >>>>> > When the CacheManager is stopped, if the index storage caches are >>>>> > stopped first, then the indexed cache is stopped, this might need to >>>>> > flush/close some pending state on the index and this results in an >>>>> > illegal operation as the storate is shut down already. >>>>> > >>>>> > We could either implement a complex dependency graph, or add a method >>>>> > like: >>>>> > >>>>> > >>>>> > boolean incRef(); >>>>> > >>>>> > on AdvancedCache. >>>>> > >>>>> > when the Cache#close() method is invoked, this will do an internal >>>>> > decrement, and only when hitting zero it will really close the cache. >>> >>> Unfortunately this won't work except in a simple dependency case (you >>> depend on a cache, but no cache can depend on you). >>> >>> Say you have 3 caches (C1, C2, C3). >>> >>> The case is C2 depends on C1 and C3 depends on C2. In this case both >>> C1 and C2 would have a ref count value of 1 and C3 would have 0. This >>> would allow for C1 and C2 to both be eligible to be closed during the >>> same iteration. >> >> Yea people could use it the wrong way :-D > > Oh I agree, but it seems this could be a pretty simple case that a > user may not know the consequences of. The problem with this you have > to know the dependencies of the cache you are depending on as well, > which I don't think most users would want. It would be fine if only > Infinispan used it internally, or at least it should :) Right, I expect this to be used at SPI level: other frameworks integrating still need a stable contract but that doesn't mean all of the SPI needs to be easy, maybe documented only in an appendix, etc.. >> But you can increment in different patterns than what you described to >> model a full graph: the important point is to allow users to define an >> order in *some* way. >> >> >>> I think if we started doing dependencies we would really need to have >>> some sort of graph to have anything more than the simple case. >>> >>> Do we know of other use cases where we may want a dependency graph >>> explicitly? It seems what you want is solvable with what is in place, >>> it just has a bug :( >> >> True, for my case a two-phases would be good enough *generally >> speaking* as we don't expect people to index stuff in a Cache which is >> also used to store an index for a different Cache, but that's a >> "legal" configuration. >> Applying Muprhy's law, that means someone will try it out and I'd >> rather be safe about that. >> >> It just so happens that the counter proposal is both trivial and also >> can handle a quite long ordering chain. >> >> I don't understand how it's solvable "with what's in place", could you >> elaborate? > > I meant that you could use the ModuleLifecycle callback that Galder > mentioned in the query module to close any caches that are needed > before the manager starts shutting down others. However until the > mentioned bug is fixed it won't quite work. When I said "with what is > in place" I meant more that we wouldn't have to design a new > implementation to support your use case. The pattern Galder suggested implies some kind of counting right ;-) just that he suggests I could implement my own.

But when you close my module (Query), it might be too late.. or too early, as there might be other users of the index cache. So you're saying I need to build this in the Lucene Directory module? That doesn't work either as the Lucene Directory should not depend on Query, nor it can tell which module is using it, but more importantly this module isn't necessarily used exclusively by the Query module.

So we're back to square one, i.e. some kind of general-purpose counting. -- Sanne _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On 26 August 2014 20:29, William Burns <mudokonman(a)gmail.com&gt; wrote: > On Tue, Aug 26, 2014 at 3:17 PM, Sanne Grinovero <sanne(a)infinispan.org&gt; wrote: >> On 26 August 2014 19:38, William Burns <mudokonman(a)gmail.com&gt; wrote: >>> On Tue, Aug 26, 2014 at 2:23 PM, Sanne Grinovero <sanne(a)infinispan.org&gt; wrote: >>>> On 26 August 2014 18:38, William Burns <mudokonman(a)gmail.com&gt; wrote: >>>>> On Mon, Aug 25, 2014 at 3:46 AM, Dan Berindei < dan.berindei(a)gmail.com&gt; wrote: >>>>>> >>>>>> >>>>>> >>>>>> On Mon, Aug 25, 2014 at 10:26 AM, Galder Zamarreño < galder(a)redhat.com&gt; >>>>>> wrote: >>>>>>> >>>>>>> >>>>>>> On 15 Aug 2014, at 15:55, Dan Berindei <dan.berindei(a)gmail.com&gt; wrote: >>>>>>> >>>>>>> > It looks to me like you actually want a partial order between caches on >>>>>>> > shutdown, so why not declare an explicit dependency (e.g. >>>>>>> > manager.stopOrder(before, after)? We could even throw an exception if the >>>>>>> > user tries to stop a cache manually in the wrong order (e.g. >>>>>>> > TestingUtil.killCacheManagers). >>>>>>> > >>>>>>> > Alternatively, we could add an event CacheManagerStopEvent(pre=true) at >>>>>>> > the cache manager level that is invoked before any cache is stopped, and you >>>>>>> > could close all the indexes in that listener. The event could even be at the >>>>>>> > cache level, if it would make things easier. >>>>> >>>>> I think something like this would be the simplest for now especially, >>>>> how this is done though we can still decide. >>>>> >>>>>>> >>>>>>> Not sure you need the listener event since we already have lifecycle event >>>>>>> callbacks for external modules. >>>>>>> >>>>>>> IOW, couldn’t you do this cache stop ordering with an implementation of >>>>>>> org.infinispan.lifecycle.ModuleLifecycle? On cacheStarting, you could maybe >>>>>>> track each started cache and give it a priority, and then on >>>>>>> cacheManagerStopping use that priority to close caches. Note: I’ve not >>>>>>> tested this and I don’t know if the callbacks happen at the right time to >>>>>>> allow this. Just thinking out loud. >>>>> >>>>> +1 this is a nice use of what is already in place. The only issue I >>>>> see here is that there is no ordering of the lifecycle callbacks if >>>>> you had more than 1 callback, which could cause issues if users wanted >>>>> to reference certain caches. >>>>> >>>>>>> >>>>>> >>>>>> Unfortunately ModuleLifecycle.cacheManagerStopping is only called _after_ >>>>>> all the caches have been stopped. >>>>> >>>>> This seems like a bug, not very nice for ordering of callback methods. >>>>> >>>>>> >>>>>> >>>>>>> >>>>>>> Cheers, >>>>>>> >>>>>>> > >>>>>>> > Cheers >>>>>>> > Dan >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>> > On Fri, Aug 15, 2014 at 3:29 PM, Sanne Grinovero < sanne(a)infinispan.org&gt; >>>>>>> > wrote: >>>>>>> > The goal being to resolve ISPN-4561, I was thinking to expose a very >>>>>>> > simple reference counter in the AdvancedCache API. >>>>>>> > >>>>>>> > As you know the Query module - which triggers on indexed caches - can >>>>>>> > use the Infinispan Lucene Directory to store its indexes in a >>>>>>> > (different) Cache. >>>>>>> > When the CacheManager is stopped, if the index storage caches are >>>>>>> > stopped first, then the indexed cache is stopped, this might need to >>>>>>> > flush/close some pending state on the index and this results in an >>>>>>> > illegal operation as the storate is shut down already. >>>>>>> > >>>>>>> > We could either implement a complex dependency graph, or add a method >>>>>>> > like: >>>>>>> > >>>>>>> > >>>>>>> > boolean incRef(); >>>>>>> > >>>>>>> > on AdvancedCache. >>>>>>> > >>>>>>> > when the Cache#close() method is invoked, this will do an internal >>>>>>> > decrement, and only when hitting zero it will really close the cache. >>>>> >>>>> Unfortunately this won't work except in a simple dependency case (you >>>>> depend on a cache, but no cache can depend on you). >>>>> >>>>> Say you have 3 caches (C1, C2, C3). >>>>> >>>>> The case is C2 depends on C1 and C3 depends on C2. In this case both >>>>> C1 and C2 would have a ref count value of 1 and C3 would have 0. This >>>>> would allow for C1 and C2 to both be eligible to be closed during the >>>>> same iteration. >>>> >>>> Yea people could use it the wrong way :-D >>> >>> Oh I agree, but it seems this could be a pretty simple case that a >>> user may not know the consequences of. The problem with this you have >>> to know the dependencies of the cache you are depending on as well, >>> which I don't think most users would want. It would be fine if only >>> Infinispan used it internally, or at least it should :) >> >> Right, I expect this to be used at SPI level: other frameworks >> integrating still need a stable contract but that doesn't mean all of >> the SPI needs to be easy, maybe documented only in an appendix, etc.. >> >> >>>> But you can increment in different patterns than what you described to >>>> model a full graph: the important point is to allow users to define an >>>> order in *some* way. >>>> >>>> >>>>> I think if we started doing dependencies we would really need to have >>>>> some sort of graph to have anything more than the simple case. >>>>> >>>>> Do we know of other use cases where we may want a dependency graph >>>>> explicitly? It seems what you want is solvable with what is in place, >>>>> it just has a bug :( >>>> >>>> True, for my case a two-phases would be good enough *generally >>>> speaking* as we don't expect people to index stuff in a Cache which is >>>> also used to store an index for a different Cache, but that's a >>>> "legal" configuration. >>>> Applying Muprhy's law, that means someone will try it out and I'd >>>> rather be safe about that. >>>> >>>> It just so happens that the counter proposal is both trivial and also >>>> can handle a quite long ordering chain. >>>> >>>> I don't understand how it's solvable "with what's in place", could you >>>> elaborate? >>> >>> I meant that you could use the ModuleLifecycle callback that Galder >>> mentioned in the query module to close any caches that are needed >>> before the manager starts shutting down others. However until the >>> mentioned bug is fixed it won't quite work. When I said "with what is >>> in place" I meant more that we wouldn't have to design a new >>> implementation to support your use case. >> >> The pattern Galder suggested implies some kind of counting right ;-) >> just that he suggests I could implement my own. > > Not necessarily, could you not just when a cache starts check it's > configuration and if it has indexing enabled keep a reference to it. > Then when the cache manager is stopping you stop those caches before > returning? I suspect that if we allow extension points to shut down other components eagerly it's going to be a mess. I can't reliably track if and which other components (and user code) are still using it?

>> But when you close my module (Query), it might be too late.. or too >> early, as there might be other users of the index cache. So you're >> saying I need to build this in the Lucene Directory module? >> That doesn't work either as the Lucene Directory should not depend on >> Query, nor it can tell which module is using it, but more importantly >> this module isn't necessarily used exclusively by the Query module. > > This wouldn't be fired when the module is closed but rather as a > notification that the cache manager is about to begin its stop > sequence. So I would eagerly shut down an indexed cache from a module which might have been started as its dependant (or not). What about other "in flight" operations happening on that Cache, we let them blow up even if technically we didn't shut down yet? Sorry for playing devil's advocate, but it seems very wrong.

BTW I omitted it so far as "execution complexity" shouldn't be an excuse for an inferior solution, but it's worth to keep in mind that some of these resources are managed by Hibernate Search and you really can't introduce hard dependencies to it so the solution we're aiming at needs to be built and supported by infinispan-core exclusively, which has to expose this as an SPI.

That said, I just wanted to explain the problem and propose a solution but have no longer time for this so any volunteer taking it? https://issues.jboss.org/browse/ISPN-4561 TiA -- Sanne _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

Thanks, this actually has multiple issues currently: - the default cache is stopped last (why ?) - some "service" caches need to be handled manually: e.g. the registry and the topology cache. A generic ref counting system would be a great improvement Tristan On 15/08/14 14:29, Sanne Grinovero wrote: > The goal being to resolve ISPN-4561, I was thinking to expose a very > simple reference counter in the AdvancedCache API. > > As you know the Query module - which triggers on indexed caches - can > use the Infinispan Lucene Directory to store its indexes in a > (different) Cache. > When the CacheManager is stopped, if the index storage caches are > stopped first, then the indexed cache is stopped, this might need to > flush/close some pending state on the index and this results in an > illegal operation as the storate is shut down already. > > We could either implement a complex dependency graph, or add a method like: > > > boolean incRef(); > > on AdvancedCache. > > when the Cache#close() method is invoked, this will do an internal > decrement, and only when hitting zero it will really close the cache. > > A CacheManager shutdown will loop through all caches, and invoke > close() on all of them; the close() method should return something so > that the CacheManager shutdown loop understand if it really did close > all caches or if not, in which case it will loop again through all > caches, and loops until all cache instances are really closed. > The return type of "close()" doesn't necessarily need to be exposed on > public API, it could be an internal only variant. > > Could we do this? > > --Sanne > _______________________________________________ > 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