On Tue, Jan 2, 2018 at 8:11 AM Romain Manni-Bucau <[email protected]> wrote:
> 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. > 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 <[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 >>>>>>>>>> >>>>>>>>>> >>>> >>>
