Romain, I'm going to top post, because I don't want to keep this back and forth going.
When I set out and declared I was planning to start Safeguard, I made it very clear I was going to use Failsafe as a dependency [1]. If you feel strongly that this is a blocker, we should make sure that's clear and any factual concerns with its use are understood. Failsafe is a library that's been through the ringer and has 2.5 years of effort behind it. We're not going to replace it in two weeks. John [1]: https://lists.apache.org/thread.html/6c550a5fd715394e82c0b337b96aec5871b99b89e6e43911d1e1c715@%3Cdev.geronimo.apache.org%3E On Tue, Jan 2, 2018 at 10:05 AM Romain Manni-Bucau <[email protected]> wrote: > 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 >>>>>>>>>>>> >>>>>>>>>>>> >>>>>> >>>>>
