geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "John D. Ament" <johndam...@apache.org>
Subject Re: Planning to cut Config & Safeguard This Week
Date Tue, 02 Jan 2018 14:01:38 GMT
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.


>
>
>>
>>
>>> 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.


>
>
>>
>>
>>> 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."


>
>
>>
>>
>>>
>>> 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