geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Romain Manni-Bucau <rmannibu...@gmail.com>
Subject Re: Planning to cut Config & Safeguard This Week
Date Thu, 04 Jan 2018 05:58:35 GMT
Le 4 janv. 2018 04:24, "John D. Ament" <john.d.ament@gmail.com> a écrit :

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.


This is true John and I think I lentionned somewhere we should drop it at
some point. Also 2.5 years of experience doesnt mean much in practise, will
not detail it to not cite any mainstream lib but Im sure you got it ;).

To detail this concern: all we build in G - EE or MP is likely used as a
container lib at some point - not only an app lib. If the stack is not
light and stay very small and self made you always end up with issues and
require users to shade which breaks part of the usage (contextual plugins
etc).

Indeed jigsaw doesnt help since it is only at jvm level and doesnt concern
much containers.

To repeat what I said: Im ok to release like that but I want to make sure
we aim to drop it. If the projects can merge it is awesome, otherwise we
will just do it ourselves, no blockers.



John

[1]: https://lists.apache.org/thread.html/6c550a5fd715394e82c0b337b96aec
5871b99b89e6e43911d1e1c715@%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