Planning to cut Config & Safeguard This Week

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

Planning to cut Config & Safeguard This Week

John D. Ament
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
Reply | Threaded
Open this post in threaded view
|

Re: Planning to cut Config & Safeguard This Week

Mark Struberg
+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?

LieGrue,
strub

> Am 01.01.2018 um 16:34 schrieb John D. Ament <[hidden email]>:
>
> 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

Reply | Threaded
Open this post in threaded view
|

Re: Planning to cut Config & Safeguard This Week

John D. Ament


On Mon, Jan 1, 2018 at 11:10 AM Mark Struberg <[hidden email]> 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 <[hidden email]>:
>
> 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

Reply | Threaded
Open this post in threaded view
|

Re: Planning to cut Config & Safeguard This Week

Romain Manni-Bucau
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" <[hidden email]> a écrit :


On Mon, Jan 1, 2018 at 11:10 AM Mark Struberg <[hidden email]> 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 <[hidden email]>:
>
> 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

Reply | Threaded
Open this post in threaded view
|

Re: Planning to cut Config & Safeguard This Week

John D. Ament
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" <[hidden email]> 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" <[hidden email]> a écrit :


On Mon, Jan 1, 2018 at 11:10 AM Mark Struberg <[hidden email]> 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 <[hidden email]>:
>
> 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

Reply | Threaded
Open this post in threaded view
|

Re: Planning to cut Config & Safeguard This Week

Romain Manni-Bucau
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.

Le 1 janv. 2018 18:51, "John D. Ament" <[hidden email]> 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" <[hidden email]> 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" <[hidden email]> a écrit :


On Mon, Jan 1, 2018 at 11:10 AM Mark Struberg <[hidden email]> 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 <[hidden email]>:
>
> 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

Reply | Threaded
Open this post in threaded view
|

Re: Planning to cut Config & Safeguard This Week

John D. Ament


On Mon, Jan 1, 2018 at 2:36 PM Romain Manni-Bucau <[hidden email]> 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" <[hidden email]> 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" <[hidden email]> 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" <[hidden email]> a écrit :


On Mon, Jan 1, 2018 at 11:10 AM Mark Struberg <[hidden email]> 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 <[hidden email]>:
>
> 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

Reply | Threaded
Open this post in threaded view
|

Re: Planning to cut Config & Safeguard This Week

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 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 |  Blog | Old BlogGithub | LinkedIn

2018-01-01 22:59 GMT+01:00 John D. Ament <[hidden email]>:


On Mon, Jan 1, 2018 at 2:36 PM Romain Manni-Bucau <[hidden email]> 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" <[hidden email]> 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" <[hidden email]> 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" <[hidden email]> a écrit :


On Mon, Jan 1, 2018 at 11:10 AM Mark Struberg <[hidden email]> 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 <[hidden email]>:
>
> 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


Reply | Threaded
Open this post in threaded view
|

Re: Planning to cut Config & Safeguard This Week

Romain Manni-Bucau
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?
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.
4. I updated the config facade to not enforce [config] dependency, can you review please?
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?
6. do we need to depend in failsafe lib? can't we do it ourself?
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.

Note linked to the impl: anyone knows why ConfigFacade is a class and not an interface? Abuse?

What do you think?



Romain Manni-Bucau
@rmannibucau |  Blog | Old BlogGithub | LinkedIn

2018-01-02 8:50 GMT+01:00 Romain Manni-Bucau <[hidden email]>:
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 |  Blog | Old BlogGithub | LinkedIn

2018-01-01 22:59 GMT+01:00 John D. Ament <[hidden email]>:


On Mon, Jan 1, 2018 at 2:36 PM Romain Manni-Bucau <[hidden email]> 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" <[hidden email]> 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" <[hidden email]> 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" <[hidden email]> a écrit :


On Mon, Jan 1, 2018 at 11:10 AM Mark Struberg <[hidden email]> 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 <[hidden email]>:
>
> 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



Reply | Threaded
Open this post in threaded view
|

Re: Planning to cut Config & Safeguard This Week

Romain Manni-Bucau
PS: pushed the system properties handling for the config


Romain Manni-Bucau
@rmannibucau |  Blog | Old BlogGithub | LinkedIn

2018-01-02 9:16 GMT+01:00 Romain Manni-Bucau <[hidden email]>:
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?
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.
4. I updated the config facade to not enforce [config] dependency, can you review please?
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?
6. do we need to depend in failsafe lib? can't we do it ourself?
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.

Note linked to the impl: anyone knows why ConfigFacade is a class and not an interface? Abuse?

What do you think?



Romain Manni-Bucau
@rmannibucau |  Blog | Old BlogGithub | LinkedIn

2018-01-02 8:50 GMT+01:00 Romain Manni-Bucau <[hidden email]>:
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 |  Blog | Old BlogGithub | LinkedIn

2018-01-01 22:59 GMT+01:00 John D. Ament <[hidden email]>:


On Mon, Jan 1, 2018 at 2:36 PM Romain Manni-Bucau <[hidden email]> 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" <[hidden email]> 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" <[hidden email]> 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" <[hidden email]> a écrit :


On Mon, Jan 1, 2018 at 11:10 AM Mark Struberg <[hidden email]> 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 <[hidden email]>:
>
> 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




Reply | Threaded
Open this post in threaded view
|

Re: Planning to cut Config & Safeguard This Week

John D. Ament
In reply to this post by Romain Manni-Bucau


On Tue, Jan 2, 2018 at 3:17 AM Romain Manni-Bucau <[hidden email]> 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.
 
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.
 
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.
 
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.
 

What do you think?



Romain Manni-Bucau
@rmannibucau |  Blog | Old BlogGithub | LinkedIn

2018-01-02 8:50 GMT+01:00 Romain Manni-Bucau <[hidden email]>:
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 |  Blog | Old BlogGithub | LinkedIn

2018-01-01 22:59 GMT+01:00 John D. Ament <[hidden email]>:


On Mon, Jan 1, 2018 at 2:36 PM Romain Manni-Bucau <[hidden email]> 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" <[hidden email]> 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" <[hidden email]> 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" <[hidden email]> a écrit :


On Mon, Jan 1, 2018 at 11:10 AM Mark Struberg <[hidden email]> 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 <[hidden email]>:
>
> 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



Reply | Threaded
Open this post in threaded view
|

Re: Planning to cut Config & Safeguard This Week

Romain Manni-Bucau


2018-01-02 13:02 GMT+01:00 John D. Ament <[hidden email]>:


On Tue, Jan 2, 2018 at 3:17 AM Romain Manni-Bucau <[hidden email]> 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 |  Blog | Old BlogGithub | LinkedIn

2018-01-02 8:50 GMT+01:00 Romain Manni-Bucau <[hidden email]>:
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 |  Blog | Old BlogGithub | LinkedIn

2018-01-01 22:59 GMT+01:00 John D. Ament <[hidden email]>:


On Mon, Jan 1, 2018 at 2:36 PM Romain Manni-Bucau <[hidden email]> 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" <[hidden email]> 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" <[hidden email]> 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" <[hidden email]> a écrit :


On Mon, Jan 1, 2018 at 11:10 AM Mark Struberg <[hidden email]> 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 <[hidden email]>:
>
> 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




Reply | Threaded
Open this post in threaded view
|

Re: Planning to cut Config & Safeguard This Week

John D. Ament


On Tue, Jan 2, 2018 at 8:11 AM Romain Manni-Bucau <[hidden email]> wrote:
2018-01-02 13:02 GMT+01:00 John D. Ament <[hidden email]>:


On Tue, Jan 2, 2018 at 3:17 AM Romain Manni-Bucau <[hidden email]> 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 |  Blog | Old BlogGithub | LinkedIn

2018-01-02 8:50 GMT+01:00 Romain Manni-Bucau <[hidden email]>:
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 |  Blog | Old BlogGithub | LinkedIn

2018-01-01 22:59 GMT+01:00 John D. Ament <[hidden email]>:


On Mon, Jan 1, 2018 at 2:36 PM Romain Manni-Bucau <[hidden email]> 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" <[hidden email]> 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" <[hidden email]> 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" <[hidden email]> a écrit :


On Mon, Jan 1, 2018 at 11:10 AM Mark Struberg <[hidden email]> 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 <[hidden email]>:
>
> 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



Reply | Threaded
Open this post in threaded view
|

Re: Planning to cut Config & Safeguard This Week

Romain Manni-Bucau


2018-01-02 15:01 GMT+01:00 John D. Ament <[hidden email]>:


On Tue, Jan 2, 2018 at 8:11 AM Romain Manni-Bucau <[hidden email]> wrote:
2018-01-02 13:02 GMT+01:00 John D. Ament <[hidden email]>:


On Tue, Jan 2, 2018 at 3:17 AM Romain Manni-Bucau <[hidden email]> 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 |  Blog | Old BlogGithub | LinkedIn

2018-01-02 8:50 GMT+01:00 Romain Manni-Bucau <[hidden email]>:
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 |  Blog | Old BlogGithub | LinkedIn

2018-01-01 22:59 GMT+01:00 John D. Ament <[hidden email]>:


On Mon, Jan 1, 2018 at 2:36 PM Romain Manni-Bucau <[hidden email]> 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" <[hidden email]> 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" <[hidden email]> 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" <[hidden email]> a écrit :


On Mon, Jan 1, 2018 at 11:10 AM Mark Struberg <[hidden email]> 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 <[hidden email]>:
>
> 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




Reply | Threaded
Open this post in threaded view
|

Re: Planning to cut Config & Safeguard This Week

John D. Ament-2
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 <[hidden email]> wrote:
2018-01-02 15:01 GMT+01:00 John D. Ament <[hidden email]>:


On Tue, Jan 2, 2018 at 8:11 AM Romain Manni-Bucau <[hidden email]> wrote:
2018-01-02 13:02 GMT+01:00 John D. Ament <[hidden email]>:


On Tue, Jan 2, 2018 at 3:17 AM Romain Manni-Bucau <[hidden email]> 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 |  Blog | Old BlogGithub | LinkedIn

2018-01-02 8:50 GMT+01:00 Romain Manni-Bucau <[hidden email]>:
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 |  Blog | Old BlogGithub | LinkedIn

2018-01-01 22:59 GMT+01:00 John D. Ament <[hidden email]>:


On Mon, Jan 1, 2018 at 2:36 PM Romain Manni-Bucau <[hidden email]> 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" <[hidden email]> 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" <[hidden email]> 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" <[hidden email]> a écrit :


On Mon, Jan 1, 2018 at 11:10 AM Mark Struberg <[hidden email]> 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 <[hidden email]>:
>
> 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



Reply | Threaded
Open this post in threaded view
|

Re: Planning to cut Config & Safeguard This Week

Romain Manni-Bucau


Le 4 janv. 2018 04:24, "John D. Ament" <[hidden email]> a écrit :
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, will 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).

Indeed jigsaw doesnt help since it is only at jvm level and doesnt 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.



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 <[hidden email]> wrote:
2018-01-02 15:01 GMT+01:00 John D. Ament <[hidden email]>:


On Tue, Jan 2, 2018 at 8:11 AM Romain Manni-Bucau <[hidden email]> wrote:
2018-01-02 13:02 GMT+01:00 John D. Ament <[hidden email]>:


On Tue, Jan 2, 2018 at 3:17 AM Romain Manni-Bucau <[hidden email]> 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 |  Blog | Old BlogGithub | LinkedIn

2018-01-02 8:50 GMT+01:00 Romain Manni-Bucau <[hidden email]>:
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 |  Blog | Old BlogGithub | LinkedIn

2018-01-01 22:59 GMT+01:00 John D. Ament <[hidden email]>:


On Mon, Jan 1, 2018 at 2:36 PM Romain Manni-Bucau <[hidden email]> 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" <[hidden email]> 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" <[hidden email]> 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" <[hidden email]> a écrit :


On Mon, Jan 1, 2018 at 11:10 AM Mark Struberg <[hidden email]> 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 <[hidden email]>:
>
> 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




Reply | Threaded
Open this post in threaded view
|

Re: Planning to cut Config & Safeguard This Week

John D. Ament
Romain,

On Thu, Jan 4, 2018 at 12:58 AM Romain Manni-Bucau <[hidden email]> wrote:


Le 4 janv. 2018 04:24, "John D. Ament" <[hidden email]> a écrit :
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, will 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).
 
Indeed jigsaw doesnt help since it is only at jvm level and doesnt 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.
 



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 <[hidden email]> wrote:
2018-01-02 15:01 GMT+01:00 John D. Ament <[hidden email]>:


On Tue, Jan 2, 2018 at 8:11 AM Romain Manni-Bucau <[hidden email]> wrote:
2018-01-02 13:02 GMT+01:00 John D. Ament <[hidden email]>:


On Tue, Jan 2, 2018 at 3:17 AM Romain Manni-Bucau <[hidden email]> 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 |  Blog | Old BlogGithub | LinkedIn

2018-01-02 8:50 GMT+01:00 Romain Manni-Bucau <[hidden email]>:
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 |  Blog | Old BlogGithub | LinkedIn

2018-01-01 22:59 GMT+01:00 John D. Ament <[hidden email]>:


On Mon, Jan 1, 2018 at 2:36 PM Romain Manni-Bucau <[hidden email]> 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" <[hidden email]> 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" <[hidden email]> 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" <[hidden email]> a écrit :


On Mon, Jan 1, 2018 at 11:10 AM Mark Struberg <[hidden email]> 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 <[hidden email]>:
>
> 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




Reply | Threaded
Open this post in threaded view
|

Re: Planning to cut Config & Safeguard This Week

Romain Manni-Bucau


Le 6 janv. 2018 16:47, "John D. Ament" <[hidden email]> a écrit :
Romain,

On Thu, Jan 4, 2018 at 12:58 AM Romain Manni-Bucau <[hidden email]> wrote:


Le 4 janv. 2018 04:24, "John D. Ament" <[hidden email]> a écrit :
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, will 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 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.
 



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 <[hidden email]> wrote:
2018-01-02 15:01 GMT+01:00 John D. Ament <[hidden email]>:


On Tue, Jan 2, 2018 at 8:11 AM Romain Manni-Bucau <[hidden email]> wrote:
2018-01-02 13:02 GMT+01:00 John D. Ament <[hidden email]>:


On Tue, Jan 2, 2018 at 3:17 AM Romain Manni-Bucau <[hidden email]> 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 |  Blog | Old BlogGithub | LinkedIn

2018-01-02 8:50 GMT+01:00 Romain Manni-Bucau <[hidden email]>:
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 |  Blog | Old BlogGithub | LinkedIn

2018-01-01 22:59 GMT+01:00 John D. Ament <[hidden email]>:


On Mon, Jan 1, 2018 at 2:36 PM Romain Manni-Bucau <[hidden email]> 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" <[hidden email]> 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" <[hidden email]> 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" <[hidden email]> a écrit :


On Mon, Jan 1, 2018 at 11:10 AM Mark Struberg <[hidden email]> 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 <[hidden email]>:
>
> 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