geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "John D. Ament" <john.d.am...@gmail.com>
Subject Re: Planning to cut Config & Safeguard This Week
Date Thu, 04 Jan 2018 03:24:23 GMT
Romain,

I'm going to top post, because I don't want to keep this back and forth
going.

When I set out and declared I was planning to start Safeguard, I made it
very clear I was going to use Failsafe as a dependency [1].  If you feel
strongly that this is a blocker, we should make sure that's clear and any
factual concerns with its use are understood.  Failsafe is a library that's
been through the ringer and has 2.5 years of effort behind it.  We're not
going to replace it in two weeks.

John

[1]:
https://lists.apache.org/thread.html/6c550a5fd715394e82c0b337b96aec5871b99b89e6e43911d1e1c715@%3Cdev.geronimo.apache.org%3E

On Tue, Jan 2, 2018 at 10:05 AM Romain Manni-Bucau <rmannibucau@gmail.com>
wrote:

> 2018-01-02 15:01 GMT+01:00 John D. Ament <johndament@apache.org>:
>
>>
>>
>> On Tue, Jan 2, 2018 at 8:11 AM Romain Manni-Bucau <rmannibucau@gmail.com>
>> wrote:
>>
>>> 2018-01-02 13:02 GMT+01:00 John D. Ament <johndament@apache.org>:
>>>
>>>>
>>>>
>>>> On Tue, Jan 2, 2018 at 3:17 AM Romain Manni-Bucau <
>>>> rmannibucau@gmail.com> wrote:
>>>>
>>>>> Here a few feedbacks:
>>>>>
>>>>> 1. org.apache.safeguard.api.config.ConfigFacade: it looks too risky
>>>>> and wrong as impl, setInstance should fail if already set + it should
>>>>> be per "app" (classloader with fallback on parent if not in the get?
=> how
>>>>> to clean? ServletContextListener for webapps?). If not it can only work
as
>>>>> a lib embedded in an app and can't be industrialized as a container/envrt
>>>>> service
>>>>> 2. org.apache.safeguard.impl.config.MicroprofileAnnotationMapper#setInstance
>>>>> why?
>>>>>
>>>>
>>>> For both of these, the issue is the per app classloading.  WE probably
>>>> need to back it by a map.
>>>>
>>>>
>>>>> 3. org.apache.safeguard.impl.FailsafeExecutionManager#FailsafeExecutionManager()
>>>>> we should move the "instances" to cdi beans (@Inject?) and if null we
do a
>>>>> plain new no? would avoid leaking (inter app) instances and old singletons
>>>>> spread accross the app and hard to change.
>>>>>
>>>>
>>>> I don't believe CDI should be used here.  We have one convenience
>>>> constructor + a constructor an implementor can use to create the class.
>>>>
>>>
>>> Hmm, there is a CDI extension in the spec and it shouldn't use not CDI
>>> beans which would be specializable. This use case is not handled making the
>>> customization hard, not natural and error prone. This was my concern.
>>> Supporting CDI beans natively is what we should propose as a programmimng
>>> model to the end user IMHO - and it doesnt violate the spec.
>>>
>>
>> Yes, there is a CDI Extension effectively required by the spec for the
>> interceptor (not explicitly required, but since you lose the runtime
>> metadata you need it).  Yes, there should be a CDI based programming model
>> to the end user, but not sure we should provide our specific classes as CDI
>> beans.
>>
>
> Ok, I don't really care much if it is or not but I care about the fact we
> look up everything which looks like a service OOTB if CDI is available.
>
>
>>
>>
>>>
>>>
>>>>
>>>>
>>>>> 4. I updated the config facade to not enforce [config] dependency, can
>>>>> you review please?
>>>>>
>>>>
>>>> Saw it, looks fine.
>>>>
>>>>
>>>>> 5. in SPI files there are apache headers, it was common to not put
>>>>> them cause some impl were not supporting them well, do we want to strip
>>>>> them?
>>>>>
>>>>
>>>> I don't understand this statement. All source files should have Apache
>>>> headers.
>>>>
>>>
>>> No, all source files which can. a JSON file can't for instance. SPI
>>> files have been here for a while. Not a blocker for me but just think it
>>> should be mentionned. In other words: if you use another API loading the
>>> SPI files and not supporting the comments you are broken.
>>>
>>
>> Oh, ok, now I know what file you mean (the META-INF/services file).  I
>> could go either way, I have seen it both ways.  Since I'm just using a
>> ServiceLoader which handles comments I think its fine.
>>
>>
>>>
>>>
>>>>
>>>>
>>>>> 6. do we need to depend in failsafe lib? can't we do it ourself?
>>>>>
>>>>
>>>> We can definitely do it ourselves, I mentioned this early on that it
>>>> would be a dependency until we can build out a replacement.  That's why
>>>> there's API sitting on top, to allow us to create a second implementation
>>>> not based on failsafe when we're ready to.
>>>>
>>>
>>> Oki, +1 then. Do you think it would be too long to block the release?
>>>
>>
>> Yes, there's a lot of functionality baked in for usage.  If anything, I'd
>> actually like to start thinking about importing his source code and
>> maintaining it here; since he's done most of the hard work but not sure his
>> plans for maintaining it long term.
>>
>
> Can you ping him to see and have a rough idea of the plan? If we import
> the code it sounds good to release like that, if not I'd like to take 2
> weeks to see if we can drop it.
>
>
>>
>>
>>>
>>>
>>>>
>>>>
>>>>> 7. you mentionned it
>>>>> but org.apache.safeguard.impl.executionPlans.ExecutionPlanFactory#locateExecutionPlan(java.lang.reflect.Method)
>>>>> should be configurable and not a 5 threads pool, default should likely
be a
>>>>> constant pool (same rule as 2 probably) otherwise more you use the lib
more
>>>>> you have threads and can end up with an unbeliving system. In TomEE at
the
>>>>> beginning we had that - with 1 thread - and saw systems with > 1000
threads
>>>>> doing almost nothing, we switched to 1 default pool with a few threads
and
>>>>> system was as well behaving. Of course it must be customizable but default
>>>>> should be saner probably.
>>>>>
>>>>
>>>> Yes, this needs to get replaced.  I need to basically create an SPI
>>>> that creates these objects, this way people can tweak them as needed.
>>>>
>>>>
>>>>>
>>>>> Note linked to the impl: anyone knows why ConfigFacade is a class and
>>>>> not an interface? Abuse?
>>>>>
>>>>
>>>> State saving.
>>>>
>>>
>>> So an abuse to not have either a provider or a packaged scope class to
>>> hold INSTANCE, right?
>>>
>>
>> I'm not sure what you mean by "abuse."
>>
>
> Design mistake/impl leak.
>
>
>>
>>
>>>
>>>
>>>>
>>>>
>>>>>
>>>>> What do you think?
>>>>>
>>>>>
>>>>>
>>>>> 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>
>>>>>
>>>>> 2018-01-02 8:50 GMT+01:00 Romain Manni-Bucau <rmannibucau@gmail.com>:
>>>>>
>>>>>> yes and no, what is true is a java 9 lib must have module SPI and
>>>>>> META-INF/services registration*s* but you also have optional imports
so
>>>>>> this is still true. That said a fallback on system properties (hardcoded
i
>>>>>> mean) works for me. Just don't want to enforce [config] to be here.
>>>>>>
>>>>>> Looking that now, will report soon
>>>>>>
>>>>>>
>>>>>> 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>
>>>>>>
>>>>>> 2018-01-01 22:59 GMT+01:00 John D. Ament <johndament@apache.org>:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Mon, Jan 1, 2018 at 2:36 PM Romain Manni-Bucau <
>>>>>>> rmannibucau@gmail.com> wrote:
>>>>>>>
>>>>>>>> Yes, idea was to use config if there in right version and
skip it
>>>>>>>> with an info log if not. Will try to check tmr. Thanks for
the pointer.
>>>>>>>>
>>>>>>>
>>>>>>> No, it's not quite that. Honestly, with Java 9 and what not I'm
a
>>>>>>> bit worried with that kind of approach since class importing
is no longer
>>>>>>> behaving the same way.  I went with a ServiceLoader approach,
this way even
>>>>>>> app servers can come up with their own configuration mechanism
independent
>>>>>>> of MP.
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Le 1 janv. 2018 18:51, "John D. Ament" <johndament@apache.org>
a
>>>>>>>> écrit :
>>>>>>>>
>>>>>>>>> You mean for safeguard?  If so its already there.  I
do want to
>>>>>>>>> move it to a separate JAR so maybe OOTB we have a system
property backed
>>>>>>>>> version?
>>>>>>>>>
>>>>>>>>> Take a look for ConfigFacade and MicroProfileConfigFacade.
>>>>>>>>>
>>>>>>>>> On Jan 1, 2018 12:37 PM, "Romain Manni-Bucau" <
>>>>>>>>> rmannibucau@gmail.com> wrote:
>>>>>>>>>
>>>>>>>>>> Any hope to have mp config optional before? Was planning
to do it
>>>>>>>>>> before Xmas but didnt get a chance yet to code it.
Can try later this week
>>>>>>>>>> probably.
>>>>>>>>>>
>>>>>>>>>> Le 1 janv. 2018 17:19, "John D. Ament" <johndament@apache.org>
a
>>>>>>>>>> écrit :
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Mon, Jan 1, 2018 at 11:10 AM Mark Struberg
<struberg@yahoo.de>
>>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> +1 go for it!
>>>>>>>>>>>>
>>>>>>>>>>>> > Safeguard requires a Config 1.2 implementation
to run, since
>>>>>>>>>>>> Config 1.2
>>>>>>>>>>>>
>>>>>>>>>>>> Geronimo-config-1.1  is microprofile-config
1.2, right?
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Yes.  Between the bugs found in the impl and
the spec issues I
>>>>>>>>>>> saw, GConfig 1.0 ended up implementing MP Config
1.1.  I think only IBM
>>>>>>>>>>> shipped an impl of just Config 1.0.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> LieGrue,
>>>>>>>>>>>> strub
>>>>>>>>>>>>
>>>>>>>>>>>> > Am 01.01.2018 um 16:34 schrieb John
D. Ament <
>>>>>>>>>>>> johndament@apache.org>:
>>>>>>>>>>>> >
>>>>>>>>>>>> > Hey guys
>>>>>>>>>>>> >
>>>>>>>>>>>> > Just pushed up the last of the changes
for Safeguard to make
>>>>>>>>>>>> it pass Fault Tolerance 1.0's TCK.  There's
a small change I still want to
>>>>>>>>>>>> make it to allow the executor to be pluggable,
and plan to have a following
>>>>>>>>>>>> release soon that introduces more configurable
properties.
>>>>>>>>>>>> >
>>>>>>>>>>>> > With that said, I'm going to plan to
stage the Config 1.1
>>>>>>>>>>>> release tomorrow and start testing the Safeguard
release process (since
>>>>>>>>>>>> this'll be the first time we're releasing
a git repo).  Once that's
>>>>>>>>>>>> working, I'll plan to stage that release
as well.
>>>>>>>>>>>> >
>>>>>>>>>>>> > Please note - Safeguard requires a Config
1.2 implementation
>>>>>>>>>>>> to run, since Config 1.2 introduces common
sense converters (for enums in
>>>>>>>>>>>> particular) and Class converter built in.
 I didn't want to register a
>>>>>>>>>>>> custom converter.
>>>>>>>>>>>> >
>>>>>>>>>>>> > John
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>
>>>>>

Mime
View raw message