You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomee.apache.org by Bruno Baptista <br...@gmail.com> on 2018/11/22 19:08:47 UTC
Please review PR #201
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_
Re: Please review PR #201
Posted by Roberto Cortez <ra...@yahoo.com.INVALID>.
I’m ok with that.
> On 23 Nov 2018, at 14:15, Bruno Baptista <br...@gmail.com> wrote:
>
> So... Jon suggested to me that the configurable resource can be done in an additional PR by one of the new guys. I think it's an excellent ideal.
>
> If no one disagrees, I'll create a Jira with the details for it.
>
> Bruno Baptista
> https://twitter.com/brunobat_
>
>
> On 23/11/18 14:05, Roberto Cortez wrote:
>> If we are happy, I would like to merge it. I could use some of the common project setup for other work.
>>
>>> On 23 Nov 2018, at 14:01, Bruno Baptista <br...@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 <da...@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 <br...@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 <br...@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_
>>>>>
Re: Please review PR #201
Posted by Bruno Baptista <br...@gmail.com>.
So... Jon suggested to me that the configurable resource can be done in
an additional PR by one of the new guys. I think it's an excellent ideal.
If no one disagrees, I'll create a Jira with the details for it.
Bruno Baptista
https://twitter.com/brunobat_
On 23/11/18 14:05, Roberto Cortez wrote:
> If we are happy, I would like to merge it. I could use some of the common project setup for other work.
>
>> On 23 Nov 2018, at 14:01, Bruno Baptista <br...@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 <da...@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 <br...@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 <br...@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_
>>>>
Re: Please review PR #201
Posted by Roberto Cortez <ra...@yahoo.com.INVALID>.
If we are happy, I would like to merge it. I could use some of the common project setup for other work.
> On 23 Nov 2018, at 14:01, Bruno Baptista <br...@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 <da...@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 <br...@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 <br...@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_
>>>
Re: Use Managed Executor with Safeguard Fault Tolerance lib - PR #201
Posted by Bruno Baptista <br...@gmail.com>.
Hi Jon,
Not yet. I'll kick another email to kickstart that discussion.
Bruno Baptista
https://twitter.com/brunobat_
On 29/11/18 21:55, Jonathan Gallimore wrote:
> 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 <br...@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 <br...@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 <br...@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 <br...@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 <br...@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.gallimore@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 <
>>>> brunobat@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 <
>> danielsoro@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 <
>>>>>>>>>>>> brunobat@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 <
>>>> brunobat@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_
>>>>>>>>>>>>>>>
Re: Use Managed Executor with Safeguard Fault Tolerance lib - PR #201
Posted by Jonathan Gallimore <jo...@gmail.com>.
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 <br...@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 <br...@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 <br...@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 <br...@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 <br...@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.gallimore@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 <
> >> brunobat@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 <
> danielsoro@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 <
> >>>>>>>>>> brunobat@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 <
> >> brunobat@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_
> >>>>>>>>>>>>>
>
Use Managed Executor with Safeguard Fault Tolerance lib - PR #201
Posted by Bruno Baptista <br...@gmail.com>.
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 <br...@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 <br...@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 <br...@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 <br...@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.gallimore@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 <
>> brunobat@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 <danielsoro@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 <
>>>>>>>>>> brunobat@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 <
>> brunobat@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_
>>>>>>>>>>>>>
Re: Please review PR #201
Posted by Romain Manni-Bucau <rm...@gmail.com>.
@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 <br...@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 <br...@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 <br...@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 <br...@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.gallimore@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 <
> brunobat@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 <danielsoro@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 <
> >>>>>>>> brunobat@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 <
> brunobat@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_
> >>>>>>>>>>>
>
Re: Please review PR #201
Posted by Bruno Baptista <br...@gmail.com>.
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 <br...@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 <br...@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 <br...@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.gallimore@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 <br...@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 <da...@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 <
>>>>>>>> brunobat@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 <br...@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_
>>>>>>>>>>>
Re: Please review PR #201
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Le lun. 26 nov. 2018 à 14:48, Bruno Baptista <br...@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 <br...@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 <br...@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.gallimore@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 <br...@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 <da...@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 <
> >>>>>> brunobat@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 <br...@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_
> >>>>>>>>>
>
Re: Please review PR #201
Posted by Bruno Baptista <br...@gmail.com>.
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.
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.
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 <br...@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 <br...@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.gallimore@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 <br...@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 <da...@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 <
>>>>>> brunobat@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 <br...@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_
>>>>>>>>>
Re: Please review PR #201
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Le ven. 23 nov. 2018 à 16:34, Bruno Baptista <br...@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 <br...@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.gallimore@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 <br...@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 <da...@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 <
> >>>> brunobat@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 <br...@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_
> >>>>>>>
>
Re: Please review PR #201
Posted by Bruno Baptista <br...@gmail.com>.
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.
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?
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 <br...@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.gallimore@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 <br...@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 <da...@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 <
>>>> brunobat@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 <br...@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_
>>>>>>>
Re: Please review PR #201
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Le ven. 23 nov. 2018 à 15:49, Bruno Baptista <br...@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.gallimore@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 <br...@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 <da...@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 <
> >> brunobat@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 <br...@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_
> >>>>>
>
Re: Please review PR #201
Posted by Bruno Baptista <br...@gmail.com>.
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?
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?
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.gallimore@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 <br...@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 <da...@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 <
>> brunobat@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 <br...@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_
>>>>>
Re: Please review PR #201
Posted by Romain Manni-Bucau <rm...@gmail.com>.
> 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.gallimore@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 <br...@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 <da...@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 <
> brunobat@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 <br...@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_
> > >>
> >
>
Re: Please review PR #201
Posted by Jonathan Gallimore <jo...@gmail.com>.
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 <br...@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 <da...@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 <br...@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 <br...@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_
> >>
>
Re: Please review PR #201
Posted by Bruno Baptista <br...@gmail.com>.
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 <da...@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 <br...@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 <br...@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_
>>
Re: Please review PR #201
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Didnt you want to make the pool configurable and not instantiated if not
used?
Le ven. 23 nov. 2018 13:20, Daniel Cunha <da...@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 <br...@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 <br...@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_
>
Re: Please review PR #201
Posted by Daniel Cunha <da...@apache.org>.
Hey Bruno,
awesome! It really sounds good! I just push my +1 :)
Em sex, 23 de nov de 2018 às 06:44, Bruno Baptista <br...@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 <br...@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_
Re: Please review PR #201
Posted by Bruno Baptista <br...@gmail.com>.
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 <br...@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_
>>
>>
Re: Please review PR #201
Posted by Roberto Cortez <ra...@yahoo.com.INVALID>.
Cool! Thank you.
I’ll have a look.
> On 22 Nov 2018, at 19:08, Bruno Baptista <br...@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_
>
>