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