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

[Dan Berindei]

On Wed, Aug 27, 2014 at 1:28 AM, Sanne Grinovero <sanne at infinispan.org>
wrote:

> On 26 August 2014 20:29, William Burns <mudokonman at gmail.com> wrote:
> > On Tue, Aug 26, 2014 at 3:17 PM, Sanne Grinovero <sanne at infinispan.org>
> wrote:
> >> On 26 August 2014 19:38, William Burns <mudokonman at gmail.com> wrote:
> >>> On Tue, Aug 26, 2014 at 2:23 PM, Sanne Grinovero <sanne at infinispan.org>
> wrote:
> >>>> On 26 August 2014 18:38, William Burns <mudokonman at gmail.com> wrote:
> >>>>> On Mon, Aug 25, 2014 at 3:46 AM, Dan Berindei <
> dan.berindei at gmail.com> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On Mon, Aug 25, 2014 at 10:26 AM, Galder Zamarreño <
> galder at redhat.com>
> >>>>>> wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>> On 15 Aug 2014, at 15: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).
> >>>>>>> >
> >>>>>>> > 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 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.
> >>>>>
> >>>>> 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?
>
>
We know that user code isn't using it any more because the user called
CacheManager.stop(). If there are other threads accessing caches in the
cache manager, the user should expect those operations to fail.


>
> >> 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.
>

It wouldn't be any different from now - stopping the cache manager stops
all the caches, we'd just change the order. Transactional caches would wait
for in-flight transactions to finish, non-transactional caches would not
wait.

It's true that this approach does not compose very well. But as Will showed
in a prior email, your reference counting proposal doesn't compose well
either. So we either do a quick fix that supports only one level of
dependencies, or we bite the bullet and support a full dependency graph.


>
> 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.
>

I don't see the need for an additional SPI, ModuleLifecycle should be
enough.


>
> 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 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/20140827/99797d38/attachment-0001.html