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

[William Burns]

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?

>
> 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 we're back to square one, i.e. some kind of general-purpose counting.
>
> -- Sanne
>
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/infinispan-dev