[infinispan-dev] Caches need be stopped in a specific order to respect cross-cache dependencies

Mon Aug 18 08:46:39 EDT 2014

On Mon, Aug 18, 2014 at 1:56 PM, Sanne Grinovero <sanne at infinispan.org>
wrote:

> On 18 August 2014 11:33, Dan Berindei <dan.berindei at 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.justin at 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 <sanne at infinispan.org> wrote:
> >>
> >> > On 15 August 2014 14:55, Dan Berindei <dan.berindei at 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 <
> sanne at 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 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
> >> > _______________________________________________
> >> > 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
> >
> >
> >
> > _______________________________________________
> > 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/infinispan-dev/attachments/20140818/aea8718c/attachment-0001.html 