tomee-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bruno Baptista <bruno...@gmail.com>
Subject Re: Use Managed Executor with Safeguard Fault Tolerance lib - PR #201
Date Fri, 30 Nov 2018 09:50:06 GMT
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 <brunobat@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 <brunobat@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 <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
View raw message