Re: Pending patches

2021-03-23 Thread Romain Manni-Bucau 
Well it can use a singleton but from a factory method. So immediate
solution is to add a public static X getInstance();.
But as mentionned it means, to keep the pluggability we should target with
such a spi, you will enforce all other impl to use such a pattern (you
cant' just switch with -D easily since adding is easy but dropping system
props is almost impossible).
A noarg public constructor is trivial and more natural with resources IMHO
- but once again tomee can does all the work to makes it equivalent, just
requires to duplicate/wrap the impls of the SPI in tomee codebase which
sounds weird to me ("we have an impl but you need to use another one").

On a more personal note I think this pattern is no more relevant and has
more pitfalls since you enforce a static instance as soon as the class is
loaded whereas it is not needed depending the lifecycle of your main - it
is not much but still, I see it as a leak in terms of design (indeed this
one is not important and not a blocker but all implies to move to the noarg
public constructor on my side).
Maybe a habit and personal choice so would be great to have another opinion
to move forward :).

Le mar. 23 mars 2021 à 08:38, Zowalla, Richard <
richard.zowa...@hs-heilbronn.de> a écrit :

> Hi,
>
> I think, it is about the configuration flexibility in tomee's
>  definitions, which wouldn't allow the use of a singleton
> instance. Hence, the consuming project would need to implement the
> interface to make it possible. But I am not that deep as Romain in the
> TomEE codebase, so it is still a guess from my side.
>
> Gruss
> Richard
>
> Am Montag, den 22.03.2021, 23:14 +0100 schrieb Florent Guillaume:
> > Hi,
> >
> > I can drop the private constructor if you want.
> > I'm surprised it's needed though, as the default instance is already
> > used by the code if no value is provided for the timeProvider
> > parameter of TransactionImpl.
> >
> > Florent
> >
> >
> > On Mon, Mar 22, 2021 at 5:49 PM Romain Manni-Bucau <
> > rmannibu...@gmail.com> wrote:
> > > Hi Richard,
> > >
> > > I still think SystemCurrentTime should have a public noarg
> > > constructor (or just drop the private one) since it will enable
> > > tomee to fully configure dynamically the tx mgr with this new
> > > feature but otherwise +1 to apply them all.
> > >
> > > Romain Manni-Bucau
> > > @rmannibucau |  Blog | Old Blog | Github | LinkedIn | Book
> > >
> > >
> > > Le lun. 22 mars 2021 à 17:03, Zowalla, Richard <
> > > richard.zowa...@hs-heilbronn.de> a écrit :
> > > > Hi all,
> > > >
> > > > wanted to raise attention on this again. 6792 would be very nice
> > > > as we
> > > > should allow TLS/SSL protocol versions for a given mail server
> > > > instead
> > > > of falling back to some hard-coded default.
> > > >
> > > > Gruss
> > > > Richard
> > > >
> > > > Am Mittwoch, den 24.02.2021, 09:33 +0100 schrieb Romain Manni-
> > > > Bucau:
> > > > > Hi all,
> > > > >
> > > > > AFAIK we have a few pending patches to apply/issue to close:
> > > > >
> > > > > - [mail] https://issues.apache.org/jira/browse/GERONIMO-6792:
> > > > update
> > > > > some defaults and config capacity
> > > > > - [mail] https://issues.apache.org/jira/browse/GERONIMO-6801
> > > > and
> > > > > https://issues.apache.org/jira/browse/GERONIMO-6800 (setText)
> > > > > - [transaction-manager]
> > > > > https://issues.apache.org/jira/browse/GERONIMO-6805: enable to
> > > > change
> > > > > the time evaluator impl
> > > > >
> > > > > If someone else can have a review it would be great (feel free
> > > > to
> > > > > apply the patch or I can do it after).
> > > > >
> > > > > note: some of the patches are waiting for some feedback - in
> > > > > particular txmgr one, wonder about tomee  usage which
> > > > can
> > > > > need to remove the private constructor of the default impl to
> > > > enable
> > > > > to configure the impl completely.
> > > > >
> > > > > Thanks,
> > > > > Romain Manni-Bucau
> > > > > @rmannibucau |  Blog | Old Blog | Github | LinkedIn | Book
> > > >
> >
> >
> --
> Richard Zowalla, M.Sc.
> Research Associate, PhD Student | Medical Informatics
>
> Hochschule Heilbronn – University of Applied Sciences
> Max-Planck-Str. 39
> D-74081 Heilbronn
> phone: +49 7131 504 6791
> mail: richard.zowa...@hs-heilbronn.de
> web: https://www.mi.hs-heilbronn.de/
>

Re: Pending patches

2021-03-23 Thread Zowalla, Richard 
Hi,

I think, it is about the configuration flexibility in tomee's
 definitions, which wouldn't allow the use of a singleton
instance. Hence, the consuming project would need to implement the
interface to make it possible. But I am not that deep as Romain in the
TomEE codebase, so it is still a guess from my side.

Gruss
Richard

Am Montag, den 22.03.2021, 23:14 +0100 schrieb Florent Guillaume:
> Hi,
> 
> I can drop the private constructor if you want.
> I'm surprised it's needed though, as the default instance is already
> used by the code if no value is provided for the timeProvider
> parameter of TransactionImpl. 
> 
> Florent
> 
> 
> On Mon, Mar 22, 2021 at 5:49 PM Romain Manni-Bucau <
> rmannibu...@gmail.com> wrote:
> > Hi Richard,
> > 
> > I still think SystemCurrentTime should have a public noarg
> > constructor (or just drop the private one) since it will enable
> > tomee to fully configure dynamically the tx mgr with this new
> > feature but otherwise +1 to apply them all.
> > 
> > Romain Manni-Bucau
> > @rmannibucau |  Blog | Old Blog | Github | LinkedIn | Book
> > 
> > 
> > Le lun. 22 mars 2021 à 17:03, Zowalla, Richard <
> > richard.zowa...@hs-heilbronn.de> a écrit :
> > > Hi all,
> > > 
> > > wanted to raise attention on this again. 6792 would be very nice
> > > as we
> > > should allow TLS/SSL protocol versions for a given mail server
> > > instead
> > > of falling back to some hard-coded default.
> > > 
> > > Gruss
> > > Richard
> > > 
> > > Am Mittwoch, den 24.02.2021, 09:33 +0100 schrieb Romain Manni-
> > > Bucau:
> > > > Hi all,
> > > > 
> > > > AFAIK we have a few pending patches to apply/issue to close:
> > > > 
> > > > - [mail] https://issues.apache.org/jira/browse/GERONIMO-6792:
> > > update
> > > > some defaults and config capacity
> > > > - [mail] https://issues.apache.org/jira/browse/GERONIMO-6801
> > > and 
> > > > https://issues.apache.org/jira/browse/GERONIMO-6800 (setText)
> > > > - [transaction-manager] 
> > > > https://issues.apache.org/jira/browse/GERONIMO-6805: enable to
> > > change
> > > > the time evaluator impl
> > > > 
> > > > If someone else can have a review it would be great (feel free
> > > to
> > > > apply the patch or I can do it after).
> > > > 
> > > > note: some of the patches are waiting for some feedback - in
> > > > particular txmgr one, wonder about tomee  usage which
> > > can
> > > > need to remove the private constructor of the default impl to
> > > enable
> > > > to configure the impl completely.
> > > > 
> > > > Thanks,
> > > > Romain Manni-Bucau
> > > > @rmannibucau |  Blog | Old Blog | Github | LinkedIn | Book
> > > 
> 
> 
-- 
Richard Zowalla, M.Sc.
Research Associate, PhD Student | Medical Informatics

Hochschule Heilbronn – University of Applied Sciences
Max-Planck-Str. 39 
D-74081 Heilbronn 
phone: +49 7131 504 6791
mail: richard.zowa...@hs-heilbronn.de
web: https://www.mi.hs-heilbronn.de/ 


smime.p7s
Description: S/MIME cryptographic signature

Re: Pending patches

2021-03-23 Thread Romain Manni-Bucau 
Le lun. 22 mars 2021 à 23:15, Florent Guillaume  a
écrit :

> Hi,
>
> I can drop the private constructor if you want.
> I'm surprised it's needed though, as the default instance is already used
> by the code if no value is provided for the timeProvider parameter of
> TransactionImpl.
>

Got that from your comment on the ticket but tomee makes bean wiring of the
tomee.xml/resources.xml configurable, this means you can make the impl of
this bean configurable.
TomEE will support a factory method (getInstance like) or constructor args
but then it means other impls will need that as well whereas they would
likely just use a "new" instantiation IMHO.
So to ensure you can switch the impl just with a -D=MyImpl or
-Dxxx=o.a.gSystemCurrentTime (I'm simplifying names and overall system
;)), being able to instantiate an instance of the default makes sense to me.
This does not mean your default wiring is not correct - it is, it is just
not aligned on some configuration mecanism.
Why i asked tomee dev on the ticket is that tomee can wrap
SystemCurrentTime to make it instantiable so wondered if it is what is
expected vs having it instantiable OOTB.

Hope it makes more sense this way.


>
> Florent
>
>
> On Mon, Mar 22, 2021 at 5:49 PM Romain Manni-Bucau 
> wrote:
>
>> Hi Richard,
>>
>> I still think SystemCurrentTime should have a public noarg constructor
>> (or just drop the private one) since it will enable tomee to fully
>> configure dynamically the tx mgr with this new feature but otherwise +1 to
>> apply them all.
>>
>> Romain Manni-Bucau
>> @rmannibucau  |  Blog
>>  | Old Blog
>>  | Github
>>  | LinkedIn
>>  | Book
>> 
>>
>>
>> Le lun. 22 mars 2021 à 17:03, Zowalla, Richard <
>> richard.zowa...@hs-heilbronn.de> a écrit :
>>
>>> Hi all,
>>>
>>> wanted to raise attention on this again. 6792 would be very nice as we
>>> should allow TLS/SSL protocol versions for a given mail server instead
>>> of falling back to some hard-coded default.
>>>
>>> Gruss
>>> Richard
>>>
>>> Am Mittwoch, den 24.02.2021, 09:33 +0100 schrieb Romain Manni-Bucau:
>>> > Hi all,
>>> >
>>> > AFAIK we have a few pending patches to apply/issue to close:
>>> >
>>> > - [mail] https://issues.apache.org/jira/browse/GERONIMO-6792: update
>>> > some defaults and config capacity
>>> > - [mail] https://issues.apache.org/jira/browse/GERONIMO-6801 and
>>> > https://issues.apache.org/jira/browse/GERONIMO-6800 (setText)
>>> > - [transaction-manager]
>>> > https://issues.apache.org/jira/browse/GERONIMO-6805: enable to change
>>> > the time evaluator impl
>>> >
>>> > If someone else can have a review it would be great (feel free to
>>> > apply the patch or I can do it after).
>>> >
>>> > note: some of the patches are waiting for some feedback - in
>>> > particular txmgr one, wonder about tomee  usage which can
>>> > need to remove the private constructor of the default impl to enable
>>> > to configure the impl completely.
>>> >
>>> > Thanks,
>>> > Romain Manni-Bucau
>>> > @rmannibucau |  Blog | Old Blog | Github | LinkedIn | Book
>>>
>>>
>
> --
> [image: Nuxeo Logo] 
>
> Florent Guillaume  Head of R  [image: LinkedIn]
>  [image: Twitter]
>  [image: Github] 
>
> Nuxeo Content Services Platform. Stay ahead.
>