tomee-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Romain Manni-Bucau <rmannibu...@gmail.com>
Subject Re: Please review PR #201
Date Mon, 26 Nov 2018 13:53:01 GMT
Le lun. 26 nov. 2018 à 14:48, Bruno Baptista <brunobat@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 <brunobat@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 <brunobat@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_
> >>>>>>>>>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message