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