2018-01-02 15:01 GMT+01:00 John D. Ament <[email protected]>: > > > 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. >
Ok, I don't really care much if it is or not but I care about the fact we look up everything which looks like a service OOTB if CDI is available. > > >> >> >>> >>> >>>> 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. > Can you ping him to see and have a rough idea of the plan? If we import the code it sounds good to release like that, if not I'd like to take 2 weeks to see if we can drop it. > > >> >> >>> >>> >>>> 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." > Design mistake/impl leak. > > >> >> >>> >>> >>>> >>>> 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 >>>>>>>>>>> >>>>>>>>>>> >>>>> >>>>
