as you prefer.. so can we get this soon please ? -- Sanne
On 18 August 2014 13:46, Dan Berindei <[email protected]> wrote: > > > > On Mon, Aug 18, 2014 at 1:56 PM, Sanne Grinovero <[email protected]> > wrote: >> >> On 18 August 2014 11:33, Dan Berindei <[email protected]> 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 <[email protected]> >> > 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 <[email protected]> wrote: >> >> >> >> > On 15 August 2014 14:55, Dan Berindei <[email protected]> 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/IndexReader.html > >> >> >> >> >> 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 >> >> >> <[email protected]> >> >> >> 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 >> >> >>> [email protected] >> >> >>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >> >> >> >> >> >> >> >> >> >> >> >> _______________________________________________ >> >> >> infinispan-dev mailing list >> >> >> [email protected] >> >> >> https://lists.jboss.org/mailman/listinfo/infinispan-dev >> >> > _______________________________________________ >> >> > infinispan-dev mailing list >> >> > [email protected] >> >> > https://lists.jboss.org/mailman/listinfo/infinispan-dev >> >> >> >> >> >> _______________________________________________ >> >> infinispan-dev mailing list >> >> [email protected] >> >> https://lists.jboss.org/mailman/listinfo/infinispan-dev >> > >> > >> > >> > _______________________________________________ >> > infinispan-dev mailing list >> > [email protected] >> > https://lists.jboss.org/mailman/listinfo/infinispan-dev >> _______________________________________________ >> infinispan-dev mailing list >> [email protected] >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > > > > _______________________________________________ > infinispan-dev mailing list > [email protected] > https://lists.jboss.org/mailman/listinfo/infinispan-dev _______________________________________________ infinispan-dev mailing list [email protected] https://lists.jboss.org/mailman/listinfo/infinispan-dev
