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/29 11:47:35 UTC
Use Managed Executor with Safeguard Fault Tolerance lib - PR #201
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 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_
> >>>>>>>>>>>>>
>