<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Aug 18, 2014 at 1:56 PM, Sanne Grinovero <span dir="ltr"><<a href="mailto:sanne@infinispan.org" target="_blank">sanne@infinispan.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div>On 18 August 2014 11:33, Dan Berindei <<a href="mailto:dan.berindei@gmail.com" target="_blank">dan.berindei@gmail.com</a>> wrote:<br>
> Ales, I don't think the implementation matters that much, I was only<br>
> concerned about the API. BTW, where could I find some documentation on MSC?<br>
><br>
> Sanne, I missed something in your initial email: you mention a Cache.close()<br>
> method, did you mean Cache.stop(), or did you mean to add a new close()<br>
> method?<br>
<br>
</div>I meant stop(), sorry.<br>
<div><br>
><br>
> Cache doesn't actually define a stop() method, it inherits the stop() method<br>
> from the Lifecycle interface. So changing its semantics only for caches<br>
> would be hacky. Adding a different close() method would be better, but it<br>
> still wouldn't be my first choice...<br>
><br>
><br>
> On Sat, Aug 16, 2014 at 12:40 AM, Ales Justin <<a href="mailto:ales.justin@gmail.com" target="_blank">ales.justin@gmail.com</a>> wrote:<br>
>><br>
>> What about if you add an SPI for this?<br>
>><br>
>> e.g. this could be nicely implemented on top of WildFly's MSC<br>
>><br>
>> And by default you would keep this simple incRef,<br>
>> or some similar simple state machine we used in Microcontainer.<br>
>><br>
>> -Ales<br>
>><br>
>> On 15 Aug 2014, at 16:26, Sanne Grinovero <<a href="mailto:sanne@infinispan.org" target="_blank">sanne@infinispan.org</a>> wrote:<br>
>><br>
>> > On 15 August 2014 14:55, Dan Berindei <<a href="mailto:dan.berindei@gmail.com" target="_blank">dan.berindei@gmail.com</a>> wrote:<br>
>> >> It looks to me like you actually want a partial order between caches on<br>
>> >> shutdown, so why not declare an explicit dependency (e.g.<br>
>> >> manager.stopOrder(before, after)? We could even throw an exception if<br>
>> >> the<br>
>> >> user tries to stop a cache manually in the wrong order (e.g.<br>
>> >> TestingUtil.killCacheManagers).<br>
>> ><br>
>> > Because that's much more complex to implement?<br>
>> > incRef() seems trivial, effective and can be used by other components<br>
>> > for different patterns.<br>
><br>
><br>
> Implementing proper dependencies doesn't need to be difficult either, all we<br>
> need is to keep a list of dependants in the cache and prune the stopped<br>
> caches from it before doing the check.<br>
<br>
</div>I expect you or your team to do it, so your choice :-)<br>
I would also be careful in how you decide to spend a day(week?) vs 1h<br>
to provide a feature which is essentially the same stuff for the user.<br>
And if you go for dependency graphs, prepare to do it transactionally<br>
and concurrently..<br></blockquote><div><br></div><div>I don't see why we would need transactions for dependency graphs any more than we would need them for incRef.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><br>
> incRef might be easier to implement, but instead it seems harder to explain<br>
> to a user in the Javadoc.<br>
<br>
</div>I didn't invent incRef myself, it's common in several other projects<br>
(Lucene for one),<br>
so I expect it to be a commonly understood pattern.<br>
<br>
Also I suggested to add it only on AdvancedCache, as I agree it's<br>
"advanced" users only.<br></blockquote><div><br></div><div>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 "<span style="color:rgb(53,56,51);font-family:Arial,Helvetica,sans-serif;font-size:12px">Closes files associated with this index", while the incRef javadoc says "</span><font color="#353833" face="Arial, Helvetica, sans-serif"><span style="font-size:12px">Note that close() simply calls decRef()". I also didn't find any mention of what the initial reference count is. </span></font></div>
<div><font color="#353833" face="Arial, Helvetica, sans-serif"><span style="font-size:12px"><br></span></font></div><div><font color="#353833" face="Arial, Helvetica, sans-serif"><span style="font-size:12px">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.</span></font></div>
<div><font color="#353833" face="Arial, Helvetica, sans-serif"><span style="font-size:12px"><br></span></font></div><div><font color="#353833" face="Arial, Helvetica, sans-serif"><span style="font-size:12px">[1] <a href="http://lucene.apache.org/core/4_0_0/core/org/apache/lucene/index/IndexReader.html">http://lucene.apache.org/core/4_0_0/core/org/apache/lucene/index/IndexReader.html</a></span></font></div>
<div><font color="#353833" face="Arial, Helvetica, sans-serif"><span style="font-size:12px"><br></span></font></div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><br>
<br>
>> >> Alternatively, we could add an event CacheManagerStopEvent(pre=true) at<br>
>> >> the<br>
>> >> cache manager level that is invoked before any cache is stopped, and<br>
>> >> you<br>
>> >> could close all the indexes in that listener. The event could even be<br>
>> >> at the<br>
>> >> cache level, if it would make things easier.<br>
>> ><br>
>> > I like that more than defining explicit dependency links and it would<br>
>> > probably be good enough for this specific problem<br>
>> > but I feel like it doesn't solve similar problems with a more complex<br>
>> > dependency sequence of services.<br>
>> > Counters are effectively providing the same semantics, just that you<br>
>> > can use the pre-close pattern nesting it "count times".<br>
>> ><br>
>> > Also having ref-counting available makes it easier for users to<br>
>> > implement independent components - with an independent lifecycle -<br>
>> > which might share the same cache.<br>
><br>
><br>
> By independent components do you mean global components? That wouldn't work,<br>
> since we only start stopping global components after we have stopped all the<br>
> caches - regardless of the order in which we stop caches.<br>
<br>
</div>I didn't meant to add this stopping feature to components, but that<br>
many other components might need an entangled sequence of shutdown of<br>
Caches. </blockquote><div><br></div><div>Ok, fair enough.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><br>
><br>
> A global pre-stop event, instead, would allow global components to do stuff<br>
> before any of the caches is stopped.<br>
<br>
</div>I haven't seen any need for such a thing so far. Your call, but I<br>
don't think we are in the business of service lifecycle management and<br>
dependency injection frameworks.<br></blockquote><div><br></div><div>I think we are in that business, whether we like it or not.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Alesj is right: at best we should make this an SPI, provide a trivial<br>
implementation and leave the details to be handled by those who<br>
thought about it properly; just that the trivial counter is good<br>
enough for my needs.<br></blockquote><div><br></div><div>How would that SPI look? And how would someone be able to provide a better implementation than our "trivial" implementation?</div><div><br></div><div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span><font color="#888888"><br>
Sanne<br>
</font></span><div><div><br>
><br>
>> ><br>
>> > -- Sanne<br>
>> ><br>
>> >><br>
>> >> Cheers<br>
>> >> Dan<br>
>> >><br>
>> >><br>
>> >><br>
>> >> On Fri, Aug 15, 2014 at 3:29 PM, Sanne Grinovero <<a href="mailto:sanne@infinispan.org" target="_blank">sanne@infinispan.org</a>><br>
>> >> wrote:<br>
>> >>><br>
>> >>> The goal being to resolve ISPN-4561, I was thinking to expose a very<br>
>> >>> simple reference counter in the AdvancedCache API.<br>
>> >>><br>
>> >>> As you know the Query module - which triggers on indexed caches - can<br>
>> >>> use the Infinispan Lucene Directory to store its indexes in a<br>
>> >>> (different) Cache.<br>
>> >>> When the CacheManager is stopped, if the index storage caches are<br>
>> >>> stopped first, then the indexed cache is stopped, this might need to<br>
>> >>> flush/close some pending state on the index and this results in an<br>
>> >>> illegal operation as the storate is shut down already.<br>
>> >>><br>
>> >>> We could either implement a complex dependency graph, or add a method<br>
>> >>> like:<br>
>> >>><br>
>> >>><br>
>> >>> boolean incRef();<br>
>> >>><br>
>> >>> on AdvancedCache.<br>
>> >>><br>
>> >>> when the Cache#close() method is invoked, this will do an internal<br>
>> >>> decrement, and only when hitting zero it will really close the cache.<br>
>> >>><br>
>> >>> A CacheManager shutdown will loop through all caches, and invoke<br>
>> >>> close() on all of them; the close() method should return something so<br>
>> >>> that the CacheManager shutdown loop understand if it really did close<br>
>> >>> all caches or if not, in which case it will loop again through all<br>
>> >>> caches, and loops until all cache instances are really closed.<br>
>> >>> The return type of "close()" doesn't necessarily need to be exposed on<br>
>> >>> public API, it could be an internal only variant.<br>
>> >>><br>
>> >>><br>
>> >>> Could we do this?<br>
>> >>><br>
>> >>> --Sanne<br>
>> >>> _______________________________________________<br>
>> >>> infinispan-dev mailing list<br>
>> >>> <a href="mailto:infinispan-dev@lists.jboss.org" target="_blank">infinispan-dev@lists.jboss.org</a><br>
>> >>> <a href="https://lists.jboss.org/mailman/listinfo/infinispan-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/infinispan-dev</a><br>
>> >><br>
>> >><br>
>> >><br>
>> >> _______________________________________________<br>
>> >> infinispan-dev mailing list<br>
>> >> <a href="mailto:infinispan-dev@lists.jboss.org" target="_blank">infinispan-dev@lists.jboss.org</a><br>
>> >> <a href="https://lists.jboss.org/mailman/listinfo/infinispan-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/infinispan-dev</a><br>
>> > _______________________________________________<br>
>> > infinispan-dev mailing list<br>
>> > <a href="mailto:infinispan-dev@lists.jboss.org" target="_blank">infinispan-dev@lists.jboss.org</a><br>
>> > <a href="https://lists.jboss.org/mailman/listinfo/infinispan-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/infinispan-dev</a><br>
>><br>
>><br>
>> _______________________________________________<br>
>> infinispan-dev mailing list<br>
>> <a href="mailto:infinispan-dev@lists.jboss.org" target="_blank">infinispan-dev@lists.jboss.org</a><br>
>> <a href="https://lists.jboss.org/mailman/listinfo/infinispan-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/infinispan-dev</a><br>
><br>
><br>
><br>
> _______________________________________________<br>
> infinispan-dev mailing list<br>
> <a href="mailto:infinispan-dev@lists.jboss.org" target="_blank">infinispan-dev@lists.jboss.org</a><br>
> <a href="https://lists.jboss.org/mailman/listinfo/infinispan-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/infinispan-dev</a><br>
_______________________________________________<br>
infinispan-dev mailing list<br>
<a href="mailto:infinispan-dev@lists.jboss.org" target="_blank">infinispan-dev@lists.jboss.org</a><br>
<a href="https://lists.jboss.org/mailman/listinfo/infinispan-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/infinispan-dev</a><br>
</div></div></blockquote></div><br></div></div>