From dev-return-98392-archive-asf-public=cust-asf.ponee.io@geronimo.apache.org Sat Jan 6 18:43:06 2018 Return-Path: X-Original-To: archive-asf-public@eu.ponee.io Delivered-To: archive-asf-public@eu.ponee.io Received: from cust-asf.ponee.io (cust-asf.ponee.io [163.172.22.183]) by mx-eu-01.ponee.io (Postfix) with ESMTP id A745618062C for ; Sat, 6 Jan 2018 18:43:06 +0100 (CET) Received: by cust-asf.ponee.io (Postfix) id 9730D160C3B; Sat, 6 Jan 2018 17:43:06 +0000 (UTC) Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by cust-asf.ponee.io (Postfix) with SMTP id E5584160C19 for ; Sat, 6 Jan 2018 18:43:04 +0100 (CET) Received: (qmail 8690 invoked by uid 500); 6 Jan 2018 17:43:04 -0000 Mailing-List: contact dev-help@geronimo.apache.org; run by ezmlm Precedence: bulk list-help: list-unsubscribe: List-Post: Reply-To: dev@geronimo.apache.org List-Id: Delivered-To: mailing list dev@geronimo.apache.org Received: (qmail 8680 invoked by uid 99); 6 Jan 2018 17:43:03 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd2-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Sat, 06 Jan 2018 17:43:03 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd2-us-west.apache.org (ASF Mail Server at spamd2-us-west.apache.org) with ESMTP id 2FB031A082A for ; Sat, 6 Jan 2018 17:43:03 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd2-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 1.879 X-Spam-Level: * X-Spam-Status: No, score=1.879 tagged_above=-999 required=6.31 tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, HTML_MESSAGE=2, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H3=-0.01, RCVD_IN_MSPIKE_WL=-0.01, SPF_PASS=-0.001] autolearn=disabled Authentication-Results: spamd2-us-west.apache.org (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com Received: from mx1-lw-eu.apache.org ([10.40.0.8]) by localhost (spamd2-us-west.apache.org [10.40.0.9]) (amavisd-new, port 10024) with ESMTP id OVHysJb9FEDc for ; Sat, 6 Jan 2018 17:42:58 +0000 (UTC) Received: from mail-yw0-f177.google.com (mail-yw0-f177.google.com [209.85.161.177]) by mx1-lw-eu.apache.org (ASF Mail Server at mx1-lw-eu.apache.org) with ESMTPS id 4D7215F286 for ; Sat, 6 Jan 2018 17:42:57 +0000 (UTC) Received: by mail-yw0-f177.google.com with SMTP id y187so2911334ywd.12 for ; Sat, 06 Jan 2018 09:42:57 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to; bh=bRzIymCIxPUrOxqdzg5egbWelscdBWrLEfbquVmD1QI=; b=M6HFUYO7huaVLzezEGwOH5NXqcwpO0tPbuf2eID6oRzw+N1nT/gxr1cPccEY2Tz3kX flV7ACxb8nL99bV6IlbINC/bh/ebDqJ+NIPFe/SOPZVcSUTnGR6huiFbljrIeTYPOnJm 98z+I8F07iOvfGA482QNIC2n2ChqkpuBox9XjGmEPUltjXnTT2jbKlPTZMWdZscYZVs2 CyFYVwlvuPXvXr9aRvgRrAdnS01ulaNFWKkiqatmUNfbpTF8bO2zm2nKK6cleRPzFAGD sFHFQNmnuYxTC+UCoa7umodtb/dJ4bXgEbveg/KVMY6XLpbT/wvrnMc0FdX/1uIS9+mc ldcA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to; bh=bRzIymCIxPUrOxqdzg5egbWelscdBWrLEfbquVmD1QI=; b=jtCUjSGDsv4Lwcyb4sDjSuATQwv7NisbvwvjvynKEWzGWHVMQ+A6obu8coMVPlKKO9 9+RCXPr9evTcKysbrLs8mo5aV7RcKQPp38MzE4YRe7UjxFQvBMcZ4u2RrJK47PiL/bWM Zy7WgIVBvWq5UYbopDgHzca9EyOpE3tysbeTZF8Bd+UVEo0PgTlc4BcBRRfKUNUrqFVn q0k+rB6bCir+yHdxAxtmEsDnas5olCOjUp8aNA+5/Karc9md88DeuBX6zetjPLuH+yQS P4mN/6+X+1WvfwKt2M3wh64APecR0V00PhJyZudxBqodkaf5VzlFObwz+udxeUBF6b6z 2cIQ== X-Gm-Message-State: AKGB3mK5vsgNm2sZ6/C0qBGUikjd8MhVKQ7PRLAW+RRY5T94VwOKaUFv YHyqVCrjVp+TIimEfgv3Pnf3ubk50CuoSIPxlXQ= X-Google-Smtp-Source: ACJfBovL82BhysESc18Xn7fpQA3UziAmIujLoHZv+M5CiUKDLmK2EU/9+/Ph8yE4hk53J31ePQ94A33QvlZpkK5uz0E= X-Received: by 10.129.134.194 with SMTP id w185mr6133400ywf.84.1515260575938; Sat, 06 Jan 2018 09:42:55 -0800 (PST) MIME-Version: 1.0 Received: by 10.37.93.13 with HTTP; Sat, 6 Jan 2018 09:42:55 -0800 (PST) Received: by 10.37.93.13 with HTTP; Sat, 6 Jan 2018 09:42:55 -0800 (PST) In-Reply-To: References: <2FA73E96-8DBD-43B4-95A8-A385E75739F8@yahoo.de> From: Romain Manni-Bucau Date: Sat, 6 Jan 2018 18:42:55 +0100 Message-ID: Subject: Re: Planning to cut Config & Safeguard This Week To: dev@geronimo.apache.org Content-Type: multipart/alternative; boundary="001a114f0d285f000405621f188b" --001a114f0d285f000405621f188b Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Le 6 janv. 2018 16:47, "John D. Ament" a =C3=A9crit= : Romain, On Thu, Jan 4, 2018 at 12:58 AM Romain Manni-Bucau wrote: > > > Le 4 janv. 2018 04:24, "John D. Ament" a =C3=A9c= rit : > > 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. > > > This is true John and I think I lentionned somewhere we should drop it at > some point. Also 2.5 years of experience doesnt mean much in practise, wi= ll > not detail it to not cite any mainstream lib but Im sure you got it ;). > > To detail this concern: all we build in G - EE or MP is likely used as a > container lib at some point - not only an app lib. If the stack is not > light and stay very small and self made you always end up with issues and > require users to shade which breaks part of the usage (contextual plugins > etc). > > I think in of itself, this is a contradictory statement. Most app servers out there are doing one of two things: - Building the functionality themselves, even when an existing open source project exists that does it in a fully portable way. - Leveraging an existing component to do the work. The fact of the matter - we see both. Fujitsu, a company not affiliated at all with Apache, created Launcher as a MP implementation that is literally just GlassFish + Geronimo Config as a dependency. But it works, and it matches all of the requirements of a MP implementation. IBM, on the flip side, planned to implement MP Rest Client independent until I threw out the idea to implement it directly in CXF. They then decided to partner up to finish up an implementation within CXF. Yes, its ideal when a library has no transitive dependencies. I want to wait to get user feedback before trying to implement something special (for what it's worth, all of the app server implementations of MP have gone with using an external library; IBM -> Failsafe, Wildfly & Kumuluz -> Hystrix). I know that John and I know hystrix choice will fail if not shaded/isolated, failsafe not being that trendy yet it will fail later. The main motivation is impl is very simple so it is a win win for our lib. But lets give it a try like that. Will need a few years before it really hurts I guess. > Indeed jigsaw doesnt help since it is only at jvm level and doesnt concer= n > much containers. > > To repeat what I said: Im ok to release like that but I want to make sure > we aim to drop it. If the projects can merge it is awesome, otherwise we > will just do it ourselves, no blockers. > Ok, I'm going to give the release a go today. > > > > John > > [1]: https://lists.apache.org/thread.html/6c550a5fd715394e82c0b337b96aec > 5871b99b89e6e43911d1e1c715@%3Cdev.geronimo.apache.org%3E > > > On Tue, Jan 2, 2018 at 10:05 AM Romain Manni-Bucau > wrote: > >> 2018-01-02 15:01 GMT+01:00 John D. Ament : >> >>> >>> >>> On Tue, Jan 2, 2018 at 8:11 AM Romain Manni-Bucau >>> wrote: >>> >>>> 2018-01-02 13:02 GMT+01:00 John D. Ament : >>>> >>>>> >>>>> >>>>> On Tue, Jan 2, 2018 at 3:17 AM Romain Manni-Bucau < >>>>> rmannibucau@gmail.com> 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 t= he get? >>>>>> =3D> how to clean? ServletContextListener for webapps?). If not it c= an 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#set= Instance >>>>>> why? >>>>>> >>>>> >>>>> For both of these, the issue is the per app classloading. WE probabl= y >>>>> need to back it by a map. >>>>> >>>>> >>>>>> 3. org.apache.safeguard.impl.FailsafeExecutionManager#FailsafeExecut= ionManager() >>>>>> we should move the "instances" to cdi beans (@Inject?) and if null w= e do a >>>>>> plain new no? would avoid leaking (inter app) instances and old sing= letons >>>>>> 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 clas= s. >>>>> >>>> >>>> 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 makin= g the >>>> customization hard, not natural and error prone. This was my concern. >>>> Supporting CDI beans natively is what we should propose as a programmi= mng >>>> 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 mo= del >>> 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 w= e >> 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 st= rip >>>>>> them? >>>>>> >>>>> >>>>> I don't understand this statement. All source files should have Apach= e >>>>> 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 t= he >>>> 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 w= hy >>>>> there's API sitting on top, to allow us to create a second implementa= tion >>>>> 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 like= ly be a >>>>>> constant pool (same rule as 2 probably) otherwise more you use the l= ib 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 threa= ds and >>>>>> system was as well behaving. Of course it must be customizable but d= efault >>>>>> 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 an= d >>>>>> 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 | Blog >>>>>> | Old Blog >>>>>> | Github >>>>>> | LinkedIn >>>>>> >>>>>> >>>>>> 2018-01-02 8:50 GMT+01:00 Romain Manni-Bucau = : >>>>>> >>>>>>> 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 import= s so >>>>>>> this is still true. That said a fallback on system properties (hard= coded 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 | Blog >>>>>>> | Old Blog >>>>>>> | Github >>>>>>> | LinkedIn >>>>>>> >>>>>>> >>>>>>> 2018-01-01 22:59 GMT+01:00 John D. Ament : >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Mon, Jan 1, 2018 at 2:36 PM Romain Manni-Bucau < >>>>>>>> rmannibucau@gmail.com> 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 po= inter. >>>>>>>>> >>>>>>>> >>>>>>>> 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 ind= ependent >>>>>>>> of MP. >>>>>>>> >>>>>>>> >>>>>>>>> >>>>>>>>> Le 1 janv. 2018 18:51, "John D. Ament" a >>>>>>>>> =C3=A9crit : >>>>>>>>> >>>>>>>>>> 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 propert= y backed >>>>>>>>>> version? >>>>>>>>>> >>>>>>>>>> Take a look for ConfigFacade and MicroProfileConfigFacade. >>>>>>>>>> >>>>>>>>>> On Jan 1, 2018 12:37 PM, "Romain Manni-Bucau" < >>>>>>>>>> rmannibucau@gmail.com> 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 l= ater this >>>>>>>>>>> week probably. >>>>>>>>>>> >>>>>>>>>>> Le 1 janv. 2018 17:19, "John D. Ament" >>>>>>>>>>> a =C3=A9crit : >>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On Mon, Jan 1, 2018 at 11:10 AM Mark Struberg < >>>>>>>>>>>> struberg@yahoo.de> wrote: >>>>>>>>>>>> >>>>>>>>>>>>> +1 go for it! >>>>>>>>>>>>> >>>>>>>>>>>>> > Safeguard requires a Config 1.2 implementation to run, sinc= e >>>>>>>>>>>>> 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 < >>>>>>>>>>>>> johndament@apache.org>: >>>>>>>>>>>>> > >>>>>>>>>>>>> > Hey guys >>>>>>>>>>>>> > >>>>>>>>>>>>> > Just pushed up the last of the changes for Safeguard to mak= e >>>>>>>>>>>>> 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 ha= ve 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 proc= ess (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 implementatio= n >>>>>>>>>>>>> to run, since Config 1.2 introduces common sense converters (= for enums in >>>>>>>>>>>>> particular) and Class converter built in. I didn't want to r= egister a >>>>>>>>>>>>> custom converter. >>>>>>>>>>>>> > >>>>>>>>>>>>> > John >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>> >>>>>> > --001a114f0d285f000405621f188b Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


Le=C2=A06 janv. 2018 16:47, "John D. Ament" <johndament@apache.org> a =C3=A9cri= t=C2=A0:
= Romain,

On Thu, Jan 4, 2018 at 12:58 AM Romain Manni-Bucau <rmannibucau@gmail.com> wrote:


Le=C2=A04 j= anv. 2018 04:24, "John D. Ament" <john.d.ament@gmail.com> a =C3=A9crit= =C2=A0:
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 d= ependency [1].=C2=A0 If you feel strongly that this is a blocker, we should= make sure that's clear and any factual concerns with its use are under= stood.=C2=A0 Failsafe is a library that's been through the ringer and h= as 2.5 years of effort behind it.=C2=A0 We're not going to replace it i= n two weeks.
This is true John and I t= hink I lentionned somewhere we should drop it at some point. Also 2.5 years= of experience doesnt mean much in practise, will not detail it to not cite= any mainstream lib but Im sure you got it ;).

<= /div>
To detail this concern: all we build in G - EE or MP= is likely used as a container lib at some point - not only an app lib. If = the stack is not light and stay very small and self made you always end up = with issues and require users to shade which breaks part of the usage (cont= extual plugins etc).


I think in of itself, this is a contradictory state= ment.=C2=A0 Most app servers out there are doing one of two things:

- Building the functionality themselves, even when an exi= sting open source project exists that does it in a fully portable way.
- Leveraging an existing component to do the work.

The fact of the matter - we see both.=C2=A0 Fujitsu, a company not a= ffiliated at all with Apache, created Launcher as a MP implementation that = is literally just GlassFish=C2=A0+ Geronimo Config as a dependency.=C2=A0 B= ut it works, and it matches all of the requirements of a MP implementation.= =C2=A0 IBM, on the flip side, planned to implement MP Rest Client independe= nt until I threw out the idea to implement it directly in CXF.=C2=A0 They t= hen decided to partner up to finish up an implementation within CXF.
<= div>
Yes, its ideal when a library has no transitive dependen= cies.=C2=A0 I want to wait to get user feedback before trying to implement = something special (for what it's worth, all of the app server implement= ations of MP have gone with using an external library; IBM -> Failsafe, = Wildfly & Kumuluz -> Hystrix).
<= /div>


I know that John and I know hystrix choice will fail if not shade= d/isolated, failsafe not being that trendy yet it will fail later. The main= motivation is impl is very simple so it is a win win for our lib. But lets= give it a try like that. Will need a few years before it really hurts I gu= ess.

=C2=A0
Indeed jigsaw doesnt help since it is only at jvm level and d= oesnt concern much containers.

To repeat what I said: Im ok to release like that but I want to make= sure we aim to drop it. If the projects can merge it is awesome, otherwise= we will just do it ourselves, no blockers.
Ok, I'm going to give the release a go today.
<= div class=3D"elided-text">
=C2=A0
<= div dir=3D"auto">



John

[1]:=C2=A0htt= ps://lists.apache.org/thread.html/6c550a5fd715394e82c0b337b96aec<= wbr>5871b99b89e6e43911d1e1c715@%3Cdev.geronimo.apache.org%3E


On Tue, Jan 2, 2018 at 10:05 AM R= omain Manni-Bucau <rmannibucau@gmail.com> wrote:
2018-01-02 15:01 GMT+01:00 John D. Ament <johndament@apache.org>:


<= div class=3D"gmail_quote">
2018-01-02 13:02 GMT+01:00 John D. Ament <johndament@apache.= org>:

=
On Tue, Jan 2, 2018 a= t 3:17 AM Romain Manni-Bucau <rmannibucau@gmail.com> wrote:
Here a few feedbacks:

1.=C2=A0org.apache.safeguard.api.config.ConfigFacade: it looks too r= isky and wrong as impl,=C2=A0setInstance should fail if already set + it sho= uld be per "app" (classloader with fallback on parent if not in t= he get? =3D> how to clean? ServletContextListener for webapps?). If not = it can only work as a lib embedded in an app and can't be industrialize= d as a container/envrt service
2.=C2=A0org.apache.safeguard.impl.confi= g.MicroprofileAnnotationMapper#setInstance why?

For both of these, the issue is th= e per app classloading.=C2=A0 WE probably need to back it by a map.
=C2=A0
<= font color=3D"#000000" face=3D"DejaVu Sans Mono">3.=C2=A0org.apache.safegua= rd.impl.FailsafeExecutionManager#FailsafeExecutionManager() we sh= ould move the "instances" to cdi beans (@Inject?) and if null we = do a plain new no? would avoid leaking (inter app) instances and old single= tons spread accross the app and hard to change.

I don't believe CDI should be used here.= =C2=A0 We have one convenience constructor=C2=A0+ a constructor an implemen= tor can use to create the class.

Hmm, there is a CDI extension in the spec and it shou= ldn't use not CDI beans which would be specializable. This use case is = not handled making the customization hard, not natural and error prone. Thi= s was my concern. Supporting CDI beans natively is what we should propose a= s a programmimng model to the end user IMHO - and it doesnt violate the spe= c.

Yes, ther= e is a CDI Extension effectively required by the spec for the interceptor (= not explicitly required, but since you lose the runtime metadata you need i= t).=C2=A0 Yes, there should be a CDI based programming model to the end use= r, but not sure we should provide our specific classes as CDI beans.
<= /div>

<= div class=3D"gmail_extra">
Ok, I don't r= eally care much if it is or not but I care about the fact we look up everyt= hing which looks like a service OOTB if CDI is available.
=
=C2=A0
=C2=A0
= =C2=A0
=C2=A0
4. I update= d the config facade to not enforce [config] dependency, can you review plea= se?

Saw it, looks= fine.
=C2=A0
5. in SPI f= iles there are apache headers, it was common to not put them cause some imp= l were not supporting them well, do we want to strip them?

I don't understand this state= ment. All source files should have Apache headers.

No, all source files which can. a J= SON 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.

<= div>Oh, ok, now I know what file you mean (the META-INF/services file).=C2= =A0 I could go either way, I have seen it both ways.=C2=A0 Since I'm ju= st using a ServiceLoader which handles comments I think its fine.
=C2=A0
=C2=A0
= =C2=A0
6. do we need to depend in failsafe= lib? can't we do it ourself?

<= /div>
We can definitely do it ourselves, I mentioned this early = on that it would be a dependency until we can build out a replacement.=C2= =A0 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.
<= /div>

<= div class=3D"gmail_extra">
Oki, +1 then. Do = you think it would be too long to block the release?

Yes, there's a lot of function= ality baked in for usage.=C2=A0 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 th= e code it sounds good to release like that, if not I'd like to take 2 w= eeks to see if we can drop it.
=C2=A0
=C2=A0
=C2=A0
=C2= =A0
7. you mentionned it but=C2=A0org.ap= ache.safeguard.impl.executionPlans.ExecutionPlanFactory#loca= teExecutionPlan(java.lang.reflect.Method) should be configurable and n= ot 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 e= nd up with an unbeliving system. In TomEE at the beginning we had that - wi= th 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 beh= aving. Of course it must be customizable but default should be saner probab= ly.

Yes, this nee= ds to get replaced.=C2=A0 I need to basically create an SPI that creates th= ese objects, this way people can tweak them as needed.
=C2= =A0

Note linked to the impl: anyone knows w= hy ConfigFacade is a class and not an interface? Abuse?
<= /blockquote>

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 m= ean by "abuse."

Design mistake/impl leak.
=C2=A0
=C2=A0
=C2=A0
=C2=A0
=
What d= o you think?


=
Romain Manni-Bucau
@rmannibucau | =C2= =A0Blog= =C2=A0| Old = Blog |=C2=A0Github=C2=A0| LinkedIn
=

2018-01-02 = 8:50 GMT+01:00 Romain Manni-Bucau <rmannibucau@gmail.com>:
yes and no, what is t= rue 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 f= allback 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 | =C2=A0Blog=C2=A0| Old Blog |=C2= =A0Github= =C2=A0| LinkedIn
=

2018-01-01 22:59 GMT+01:00 John D. Ament <johndament@apache.org>:
=


= On Mon, Jan 1, 2018 at 2:36 PM Romain Manni-Bucau <rmannibucau@gmail.com> wrote:<= br>
Yes, idea was to = use config if there in right version and skip it with an info log if not. W= ill 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.=C2=A0 I went with a ServiceLoader appr= oach, this way even app servers can come up with their own configuration me= chanism independent of MP.
=C2=A0

Le=C2= =A01 janv. 2018 18:51, "John D. Ament" <johndament@apache.org> a =C3=A9= crit=C2=A0:
You mean for safeguard?=C2=A0 If so its already there.=C2=A0 I do= want to move it to a separate JAR so maybe OOTB we have a system property = backed version?

Take a look fo= r ConfigFacade and MicroProfileConfigFacade.

On Jan 1, 2018 12:37 PM, "Romai= n Manni-Bucau" <rmannibucau@gmail.com> wrote:
Any hope to have mp config optio= nal before? Was planning to do it before Xmas but didnt get a chance yet to= code it. Can try later this week probably.

Le=C2=A01 janv. 2018 17:19, "John D. A= ment" <j= ohndament@apache.org> a =C3=A9crit=C2=A0:


On Mon, Jan 1, 2018 at 11:10 AM Mark Struberg <struberg@yahoo.de&= gt; wrote:
+1 go for it!

> Safeguard requires a Config 1.2 implementation to run, since Config 1.= 2

Geronimo-config-1.1=C2=A0 is microprofile-config 1.2, right?

Yes.=C2=A0 Between the bugs found in the impl and the= spec issues I saw, GConfig 1.0 ended up implementing MP Config 1.1.=C2=A0 = I think only IBM shipped an impl of just Config 1.0.
=C2=A0
=

LieGrue,
strub

> Am 01.01.2018 um 16:34 schrieb John D. Ament <johndament@apache.org>:
>
> Hey guys
>
> Just pushed up the last of the changes for Safeguard to make it pass F= ault Tolerance 1.0's TCK.=C2=A0 There's a small change I still want= to make it to allow the executor to be pluggable, and plan to have a follo= wing 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).=C2=A0 Once that's w= orking, I'll plan to stage that release as well.
>
> Please note - Safeguard requires a Config 1.2 implementation to run, s= ince Config 1.2 introduces common sense converters (for enums in particular= ) and Class converter built in.=C2=A0 I didn't want to register a custo= m converter.
>
> John





--001a114f0d285f000405621f188b--