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 Tue, 02 Jan 2018 15:04:48 GMT
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