PS: pushed the system properties handling for the config
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 9:16 GMT+01:00 Romain Manni-Bucau <[email protected]>: > 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? > 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. > 4. I updated the config facade to not enforce [config] dependency, can you > review please? > 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? > 6. do we need to depend in failsafe lib? can't we do it ourself? > 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. > > Note linked to the impl: anyone knows why ConfigFacade is a class and not > an interface? 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 <[email protected]>: > >> 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 <[email protected]>: >> >>> >>> >>> On Mon, Jan 1, 2018 at 2:36 PM Romain Manni-Bucau <[email protected]> >>> 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" <[email protected]> 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" <[email protected]> >>>>> 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" <[email protected]> a >>>>>> écrit : >>>>>> >>>>>>> >>>>>>> >>>>>>> On Mon, Jan 1, 2018 at 11:10 AM Mark Struberg <[email protected]> >>>>>>> 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 < >>>>>>>> [email protected]>: >>>>>>>> > >>>>>>>> > 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 >>>>>>>> >>>>>>>> >> >
