Also merged and I'll run a local build. Was there going to be another iteration to provide some configuration on the executor service?
Jon On Thu, Nov 29, 2018 at 11:47 AM Bruno Baptista <bruno...@gmail.com> wrote: > Hi > > Can some committer decide if this is good to merge? > > https://github.com/apache/tomee/pull/201 > > Cheers > > Bruno Baptista > https://twitter.com/brunobat_ > > > On 26/11/18 16:36, Romain Manni-Bucau wrote: > > @Bruno: note that this is not what we are doing, I'm just mentionning > that > > TomEE does not need that and that there is no need to put any pressure > > either on TomEE or Geronimo in such situation since everything is good on > > both side in current state. > > > > Romain Manni-Bucau > > @rmannibucau <https://twitter.com/rmannibucau> | Blog > > <https://rmannibucau.metawerx.net/> | Old Blog > > <http://rmannibucau.wordpress.com> | Github < > https://github.com/rmannibucau> | > > LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book > > < > https://www.packtpub.com/application-development/java-ee-8-high-performance > > > > > > > > Le lun. 26 nov. 2018 à 17:05, Bruno Baptista <bruno...@gmail.com> a > écrit : > > > >> It's just that I would expect to release 1.0.1 for a mater of principle. > >> > >> I think we shouldn't throw away an already approved valid contribution. > >> > >> Bruno Baptista > >> https://twitter.com/brunobat_ > >> > >> > >> On 26/11/18 13:53, Romain Manni-Bucau wrote: > >>> Le lun. 26 nov. 2018 à 14:48, Bruno Baptista <bruno...@gmail.com> a > >> écrit : > >>>> Hi Romain, > >>>> > >>>> We are holding other work with this discussion. > >>>> > >>>> Can we agree that this is good enough for a 1st version and move on > with > >>>> a follow up PR?... It's not going to be worse than starting SE tasks > >>>> inside the container, like we have now. > >>>> > >>> As I said, while it is not released without being harnessed I'm happy > >>> without any way working for you. > >>> > >>> > >>>> Also, releasing Safegard 1.0.1 would be nice. There is unreleased code > >>>> in there that this work needs. We can live with the SNAPSHOT in the > >>>> meantime because there is no prediction of work for that SNAPSHOT. > >>>> > >>> I don't think there is anything needed, you can replace all that by a > >>> standard cdi extension if the snapshot is bothering you can use the > last > >>> release. > >>> Just veto the default and override the impl, no? > >>> > >>> > >>>> Cheers. > >>>> > >>>> Bruno Baptista > >>>> https://twitter.com/brunobat_ > >>>> > >>>> > >>>> On 23/11/18 15:41, Romain Manni-Bucau wrote: > >>>>> Le ven. 23 nov. 2018 à 16:34, Bruno Baptista <bruno...@gmail.com> a > >>>> écrit : > >>>>>> Hi Romain, > >>>>>> > >>>>>> About "The point is not the cdi bean but the executor. So high level > >> you > >>>>>> deploy an > >>>>>> app not using safeguard but it being present and you ensure the > >>>> container > >>>>>> has no executor resource instantiated (you will get one (the > >> facade))." > >>>>>> Sorry Romain, I still don't understand how the code in the PR can > >>>>>> possible affect something not using the FT API or Safeguard in > >>>> particular. > >>>>> I think the code is ok but it uses assumptions which are likely not > >>>> obvious > >>>>> and it is not tested so next commit will break it - since this code > >> must > >>>> be > >>>>> reworked anyway - and you will not see it. > >>>>> So better to ensure the build guarantee all the outcome we want for > end > >>>>> users. > >>>>> > >>>>> > >>>>>> In relation to the Managed executor... What you say makes sense but > I > >>>>>> wonder how likely it is to happen and if it's enough to hold the PR. > >> Do > >>>>>> you have a custom executor example somewhere? > >>>>>> > >>>>> We have some in the core tests you can reuse. But long story short > you > >>>> run > >>>>> your test, don't use safeguard and guarantee in @Test by looking up > the > >>>>> resource directly using internals (SystemInstance > ContainerSystem > and > >>>> so > >>>>> on) that the instance is not yet instantiated. See for a test doing > >>>> exactly > >>>>> that: > >>>>> > >> > https://github.com/apache/tomee/blob/master/container/openejb-core/src/test/java/org/apache/openejb/assembler/classic/LazyResourceTest.java#L41 > >>>>> To summarize: > >>>>> > >>>>> 1. CDI is lazy > >>>>> 2. we define the default executor as being lazy > >>>>> 3. we assume safeguard will not impact an app not using it > >>>>> > >>>>> ==> you must ensure that 3 didnt trigger an executor creation, it is > >> fine > >>>>> to rely on 1+2 (which means so "main" code) > >>>>> > >>>>> > >>>>>> Cheers > >>>>>> > >>>>>> Bruno Baptista > >>>>>> https://twitter.com/brunobat_ > >>>>>> > >>>>>> > >>>>>> On 23/11/18 15:14, Romain Manni-Bucau wrote: > >>>>>>> Le ven. 23 nov. 2018 à 15:49, Bruno Baptista <bruno...@gmail.com> > a > >>>>>> écrit : > >>>>>>>> Hi Romain, > >>>>>>>> > >>>>>>>> Thanks for your comment. > >>>>>>>> > >>>>>>>> The class doing the resource injection is lazy loaded, > specifically > >>>>>>>> /FailsafeContainerExecutionManagerProvider/. I did verify it in > >>>>>>>> development but no test was produced... And to say the truth I > >>>> wouldn't > >>>>>>>> know how to validate if a bean has already been loaded or not. Can > >> you > >>>>>>>> please provide a test example? > >>>>>>>> > >>>>>>> The point is not the cdi bean but the executor. So high level you > >>>> deploy > >>>>>> an > >>>>>>> app not using safeguard but it being present and you ensure the > >>>> container > >>>>>>> has no executor resource instantiated (you will get one (the > >> facade)). > >>>>>>> > >>>>>>>> Please explain what do you mean by "MP-fault-tolerance executor > for > >>>> that > >>>>>>>> case if noone exists". It will exist, that's the whole purpose of > >> this > >>>>>>>> PR. Can you please provide an example where a > >>>>>>>> /ManagedScheduledExecutorService/ will not be present? > >>>>>>>> > >>>>>>> You can see it as "don't let it default to a random executor". This > >> is > >>>>>> the > >>>>>>> current behavior. So here is what can happen: > >>>>>>> > >>>>>>> 1. The user doesnt use any executor -> it defaults -> it is ok > >>>>>>> 2. The user uses one or more executors for his app -> it defaults > to > >> it > >>>>>> -> > >>>>>>> it messes up the app and does not have the expected setting > >>>>>>> > >>>>>>> Case 2 is important cause it can really make it not functional and > >> even > >>>>>>> lead to locks in some cases so better to not let it happen and just > >>>>>> create > >>>>>>> a safeguard executor if > >>>>>>> the user didnt specify he wants safeguard to use the executor > >>>>>>> "'mysafeguardexecutor". > >>>>>>> > >>>>>>> This is why the config is important and I mentionned it early even > if > >>>> it > >>>>>> is > >>>>>>> not the most sexy part to do, I agree. > >>>>>>> > >>>>>>> > >>>>>>>> Cheers > >>>>>>>> > >>>>>>>> Bruno Baptista > >>>>>>>> https://twitter.com/brunobat_ > >>>>>>>> > >>>>>>>> > >>>>>>>> On 23/11/18 14:39, Romain Manni-Bucau wrote: > >>>>>>>>>> It's lazily loaded, so no worries on that regard. > >>>>>>>>> What is "it" here? :) > >>>>>>>>> > >>>>>>>>> Conretely the bean instantiation yes cause it is normal scoped > and > >>>> the > >>>>>>>>> resource too cause it is by default lazy in tomee > (service-jar.xml) > >>>> but > >>>>>>>> it > >>>>>>>>> is worth a test that prevent regression on that behavior IMHO, I > >>>> didn't > >>>>>>>>> catch on in the PR. > >>>>>>>>> > >>>>>>>>> Concretely in terms of container we can want to create a > dedicated > >>>>>>>>> MP-fault-tolerance executor for that case if noone exists and the > >>>> user > >>>>>>>>> didn't specify one cause this default behavior (cumulated with > >> tomee > >>>>>>>>> defaulting on @Resouce) will make this not reliable which is > quite > >>>>>>>>> ridiculous when you think about it for something about failt > >>>> tolerance. > >>>>>>>>> This is why it should be in before next release. Now if you do > the > >> PR > >>>>>>>> next > >>>>>>>>> week it is fine, was not to do it today but to ensure it is not > >>>> merged > >>>>>>>> and > >>>>>>>>> the enthusiasm makes it forgotten. > >>>>>>>>> > >>>>>>>>> Romain Manni-Bucau > >>>>>>>>> @rmannibucau <https://twitter.com/rmannibucau> | Blog > >>>>>>>>> <https://rmannibucau.metawerx.net/> | Old Blog > >>>>>>>>> <http://rmannibucau.wordpress.com> | Github < > >>>>>>>> https://github.com/rmannibucau> | > >>>>>>>>> LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book > >>>>>>>>> < > >> > https://www.packtpub.com/application-development/java-ee-8-high-performance > >>>>>>>>> Le ven. 23 nov. 2018 à 15:18, Jonathan Gallimore < > >>>>>>>>> jonathan.gallim...@gmail.com> a écrit : > >>>>>>>>> > >>>>>>>>>> Maybe add those config options in a second PR? What do you > think? > >>>>>>>>>> > >>>>>>>>>> Jon > >>>>>>>>>> > >>>>>>>>>> On Fri, Nov 23, 2018 at 2:01 PM Bruno Baptista < > >> bruno...@gmail.com> > >>>>>>>> wrote: > >>>>>>>>>>> Hi Romain, > >>>>>>>>>>> > >>>>>>>>>>> In the end I decided to simply use the server default, for now. > >>>>>>>>>>> > >>>>>>>>>>> It will only be used if annotations are called in the code. > It's > >>>>>> lazily > >>>>>>>>>>> loaded, so no worries on that regard. > >>>>>>>>>>> > >>>>>>>>>>> Cheers. > >>>>>>>>>>> > >>>>>>>>>>> Bruno Baptista > >>>>>>>>>>> https://twitter.com/brunobat_ > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> On 23/11/18 12:31, Romain Manni-Bucau wrote: > >>>>>>>>>>>> Didnt you want to make the pool configurable and not > >> instantiated > >>>> if > >>>>>>>>>> not > >>>>>>>>>>>> used? > >>>>>>>>>>>> > >>>>>>>>>>>> Le ven. 23 nov. 2018 13:20, Daniel Cunha < > daniels...@apache.org > >>>> a > >>>>>>>>>>> écrit : > >>>>>>>>>>>>> Hey Bruno, > >>>>>>>>>>>>> > >>>>>>>>>>>>> awesome! It really sounds good! I just push my +1 :) > >>>>>>>>>>>>> > >>>>>>>>>>>>> Em sex, 23 de nov de 2018 às 06:44, Bruno Baptista < > >>>>>>>>>> bruno...@gmail.com> > >>>>>>>>>>>>> escreveu: > >>>>>>>>>>>>> > >>>>>>>>>>>>>> Thanks! > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Additionally, I've requested a Safeguard 1.0.1 release. we > >>>>>> shouldn't > >>>>>>>>>> be > >>>>>>>>>>>>>> using snapshots. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Cheers > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Bruno Baptista > >>>>>>>>>>>>>> https://twitter.com/brunobat_ > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> On 22/11/18 19:30, Roberto Cortez wrote: > >>>>>>>>>>>>>>> Cool! Thank you. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> I’ll have a look. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> On 22 Nov 2018, at 19:08, Bruno Baptista < > >> bruno...@gmail.com> > >>>>>>>>>> wrote: > >>>>>>>>>>>>>>>> Hi! > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> I think the code is ready. Can some of you please review > >> this > >>>>>> pull > >>>>>>>>>>>>>> request: > >>>>>>>>>>>>>>>> https://github.com/apache/tomee/pull/201 > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> Related to:TOMEE-2278 < > >>>>>>>>>>>>> https://issues.apache.org/jira/browse/TOMEE-2278>- > >>>>>>>>>>>>>> Use Managed Executor with Safeguard Fault Tolerance lib > >>>>>>>>>>>>>>>> Cheers. > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> -- > >>>>>>>>>>>>>>>> Bruno Baptista > >>>>>>>>>>>>>>>> https://twitter.com/brunobat_ > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>> -- > >>>>>>>>>>>>> Daniel "soro" Cunha > >>>>>>>>>>>>> https://twitter.com/dvlc_ > >>>>>>>>>>>>> >
