On Mon, Aug 18, 2014 at 1:56 PM, Sanne Grinovero <sa...@infinispan.org> wrote:
> On 18 August 2014 11:33, Dan Berindei <dan.berin...@gmail.com> 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.jus...@gmail.com> > 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 <sa...@infinispan.org> wrote: > >> > >> > On 15 August 2014 14:55, Dan Berindei <dan.berin...@gmail.com> 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 < > sa...@infinispan.org> > >> >> 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@lists.jboss.org > >> >>> https://lists.jboss.org/mailman/listinfo/infinispan-dev > >> >> > >> >> > >> >> > >> >> _______________________________________________ > >> >> infinispan-dev mailing list > >> >> infinispan-dev@lists.jboss.org > >> >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > >> > _______________________________________________ > >> > infinispan-dev mailing list > >> > infinispan-dev@lists.jboss.org > >> > https://lists.jboss.org/mailman/listinfo/infinispan-dev > >> > >> > >> _______________________________________________ > >> infinispan-dev mailing list > >> infinispan-dev@lists.jboss.org > >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > > > > > > > > _______________________________________________ > > infinispan-dev mailing list > > infinispan-dev@lists.jboss.org > > https://lists.jboss.org/mailman/listinfo/infinispan-dev > _______________________________________________ > infinispan-dev mailing list > infinispan-dev@lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev >
_______________________________________________ infinispan-dev mailing list infinispan-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev