2018-01-02 13:02 GMT+01:00 John D. Ament <[email protected]>: > > > On Tue, Jan 2, 2018 at 3:17 AM Romain Manni-Bucau <[email protected]> > 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. > > >> 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. > > >> 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? > > >> 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? > > >> >> 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 >>>>>>>>> >>>>>>>>> >>> >>
