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