Client Commands and SecurityService

[Kirk Lund <kl...@apache.org>]

The client Commands now check with SecurityService even when security is
not configured. This has introduced a negative performance impact.

The best way to fix something like this is to tell the Command instance
when it's being constructed that is does or does not need to perform
security checks.

Unfortunately, Commands are all implemented as singletons which are very
eagerly instantiated during class loading of CommandInitializer (who
thought that was a good idea?!).

In order to fix this performance problem, I would need to get rid of these
problematic static initializer blocks that so eagerly construct the
Commands so that I can put off constructing them until AFTER the Cache is
initializing and specifically AFTER the Cache has determined if it is using
security or not.

This means I'm going to have to do some refactoring of CommandInitializer,
the Command classes, ServerConnection, AcceptorImpl, etc.

Any other approach is going to have such minimal impact on performance that
I'm not even interested in doing less than this.

From a very high level, I would change the code so that the Cache owns the
Server which owns the Command instances. In this way, the configuring of
use of security can trickle down from Cache to each Command. I would
primarily be altering static singletons, static initializers and adding
constructors.

Does anyone have a problem with me changing the above classes and
especially getting rid of the static initializers and singleton instances?

Re: Client Commands and SecurityService

[Jacob Barrett <jb...@pivotal.io>]

+1

We are attempting the same on the non-Java clients too.


-Jake

On Mon, Apr 3, 2017 at 1:30 PM Kirk Lund <kl...@apache.org> wrote:

> My primary goal is to defer instantiating the Command instances until after
> the Cache has initialized Security. I then have several options for
> handling Security within the Commands that has much less performance impact
> than it currently has. A major reconfiguration or lifecycle point, such as
> closing and then recreating the Cache should be the only source of garbage
> caused by throwing away the old instances and creating new ones.
>
> Removal of singletons primarily involves moving the instantiation code and
> the lifecycle of the class (that's a singleton) further up the layers of
> Geode code so it becomes driven by Cache initialization rather than by
> class loading. Conceptually, you would still only have one instance of
> SecurityService for the Cache, but the code that's responsible for that
> "singularity" are the collaborators that own and interact with the
> instance. This allows us to replace the instance very easily between unit
> tests or between lifecycles of the Cache (which is where a user would
> reconfigure things) or even make future changes to support multiple
> instances for some component types that are currently singletons.
>
> What I'm planning is just a first pass at reducing our dependency on
> singletons specifically to allow me to improve performance involving how
> client Commands interact with Security (when security is enabled or
> disabled).
>
> On Mon, Apr 3, 2017 at 11:02 AM, Bruce Schuchardt <bs...@pivotal.io>
> wrote:
>
> > I'm not against refactoring the command classes.  They originated from
> the
> > refactoring of a very large method that attempted to handle all client
> > operations and, consequently, are currently stateless. You can't really
> > hurt anything by creating multiple instances of them but please avoid
> > creating unnecessary garbage.
> >
> >
> >
> > Le 4/3/2017 à 10:38 AM, Galen M O'Sullivan a écrit :
> >
> >> +1 to getting rid of singletons and non-constant use of static. Also to
> >> code where the ownership semantics are clear.
> >>
> >> On Mon, Apr 3, 2017 at 10:30 AM, Jacob Barrett <jb...@pivotal.io>
> >> wrote:
> >>
> >> +1 refactor
> >>> On Mon, Apr 3, 2017 at 9:35 AM Michael William Dodge <
> mdodge@pivotal.io>
> >>> wrote:
> >>>
> >>> +1 to modular and questioning non-constant use of static
> >>>>
> >>>> On 3 Apr, 2017, at 09:27, Anthony Baker <ab...@pivotal.io> wrote:
> >>>>>
> >>>>> Using singletons leads to very monolithic systems that are hard to
> test
> >>>>>
> >>>> and hard to change.  Instead we should prefer modular services like
> Udo
> >>>> proposed.
> >>>>
> >>>>> I would go further and say that we should question any non-constant
> use
> >>>>>
> >>>> of “static”.
> >>>>
> >>>>> Anthony
> >>>>>
> >>>>> On Apr 3, 2017, at 9:01 AM, Udo Kohlmeyer <uk...@pivotal.io>
> >>>>>>
> >>>>> wrote:
> >>>>
> >>>>> Correct, that would be the definition.
> >>>>>>
> >>>>>> Yet, we find that our use of singletons within Geode is limiting to
> >>>>>>
> >>>>> say
> >>>
> >>>> that least. With the idea of wanting to be able to create/run multiple
> >>>> cache instance within the same JVM (especially for testing) a
> singleton
> >>>> will be problematic.
> >>>>
> >>>>> In addition to that, the alternative is not that hard to construct
> and
> >>>>>>
> >>>>> in many cases easier to manage.
> >>>>
> >>>>> --Udo
> >>>>>>
> >>>>>>
> >>>>>> On 4/3/17 08:57, Jinmei Liao wrote:
> >>>>>>
> >>>>>>> Isn't "that instance is reused each invocation" my understanding
> of a
> >>>>>>> "singleton"?
> >>>>>>>
> >>>>>>> On Mon, Apr 3, 2017 at 11:49 AM, Udo Kohlmeyer <
> >>>>>>>
> >>>>>> ukohlmeyer@pivotal.io>
> >>>
> >>>> wrote:
> >>>>>>>
> >>>>>>> -1 For using singletons
> >>>>>>>>
> >>>>>>>> Using a Factory pattern you can avoid having to create singletons
> in
> >>>>>>>> addition to caching created commands to avoid the recreation of
> the
> >>>>>>>> instance.
> >>>>>>>>
> >>>>>>>> The SSLConfigurationFactory is a simple example where you create
> an
> >>>>>>>> instance when required. Once an instance is created, that instance
> >>>>>>>>
> >>>>>>> is
> >>>
> >>>> reused each invocation.
> >>>>>>>>
> >>>>>>>> --Udo
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 4/3/17 08:30, Jinmei Liao wrote:
> >>>>>>>>
> >>>>>>>> I think the client commands needs to be singleton instances even
> >>>>>>>>>
> >>>>>>>> after you
> >>>>
> >>>>> change the sequence of initialization. We don't want to have each
> >>>>>>>>>
> >>>>>>>> client
> >>>>
> >>>>> operation ends up creating a new command instance, right? That
> >>>>>>>>>
> >>>>>>>> would
> >>>
> >>>> be a
> >>>>
> >>>>> more performance drag.
> >>>>>>>>>
> >>>>>>>>> On Thu, Mar 30, 2017 at 2:14 PM, Kirk Lund <kl...@apache.org>
> >>>>>>>>>
> >>>>>>>> wrote:
> >>>
> >>>> PS: I'll be writing and using JMH benchmarks to drive these
> >>>>>>>>>
> >>>>>>>> changes.
> >>>
> >>>> I'll
> >>>>
> >>>>> also create new unit tests for each of these classes that don't
> >>>>>>>>>>
> >>>>>>>>> currently
> >>>>
> >>>>> have unit tests.
> >>>>>>>>>>
> >>>>>>>>>> On Thu, Mar 30, 2017 at 10:58 AM, Kirk Lund <kl...@apache.org>
> >>>>>>>>>>
> >>>>>>>>> wrote:
> >>>>
> >>>>> The client Commands now check with SecurityService even when
> >>>>>>>>>>
> >>>>>>>>> security is
> >>>>
> >>>>> not configured. This has introduced a negative performance
> >>>>>>>>>>>
> >>>>>>>>>> impact.
> >>>
> >>>> The best way to fix something like this is to tell the Command
> >>>>>>>>>>>
> >>>>>>>>>> instance
> >>>>
> >>>>> when it's being constructed that is does or does not need to
> >>>>>>>>>>>
> >>>>>>>>>> perform
> >>>>
> >>>>> security checks.
> >>>>>>>>>>>
> >>>>>>>>>>> Unfortunately, Commands are all implemented as singletons which
> >>>>>>>>>>>
> >>>>>>>>>> are very
> >>>>
> >>>>> eagerly instantiated during class loading of CommandInitializer
> >>>>>>>>>>>
> >>>>>>>>>> (who
> >>>>
> >>>>> thought that was a good idea?!).
> >>>>>>>>>>>
> >>>>>>>>>>> In order to fix this performance problem, I would need to get
> rid
> >>>>>>>>>>>
> >>>>>>>>>> of
> >>>>
> >>>>> these
> >>>>>>>>>>
> >>>>>>>>>> problematic static initializer blocks that so eagerly construct
> >>>>>>>>>>>
> >>>>>>>>>> the
> >>>
> >>>> Commands so that I can put off constructing them until AFTER the
> >>>>>>>>>>>
> >>>>>>>>>> Cache
> >>>>
> >>>>> is
> >>>>>>>>>>> initializing and specifically AFTER the Cache has determined if
> >>>>>>>>>>>
> >>>>>>>>>> it
> >>>
> >>>> is
> >>>>
> >>>>> using
> >>>>>>>>>>
> >>>>>>>>>> security or not.
> >>>>>>>>>>>
> >>>>>>>>>>> This means I'm going to have to do some refactoring of
> >>>>>>>>>>>
> >>>>>>>>>>> CommandInitializer,
> >>>>>>>>>>
> >>>>>>>>>> the Command classes, ServerConnection, AcceptorImpl, etc.
> >>>>>>>>>>>
> >>>>>>>>>>> Any other approach is going to have such minimal impact on
> >>>>>>>>>>>
> >>>>>>>>>> performance
> >>>>
> >>>>> that I'm not even interested in doing less than this.
> >>>>>>>>>>>
> >>>>>>>>>>>  From a very high level, I would change the code so that the
> >>>>>>>>>>> Cache
> >>>>>>>>>>>
> >>>>>>>>>> owns
> >>>>
> >>>>> the
> >>>>>>>>>>
> >>>>>>>>>> Server which owns the Command instances. In this way, the
> >>>>>>>>>>>
> >>>>>>>>>> configuring of
> >>>>
> >>>>> use of security can trickle down from Cache to each Command. I
> >>>>>>>>>>>
> >>>>>>>>>> would
> >>>>
> >>>>> primarily be altering static singletons, static initializers and
> >>>>>>>>>>>
> >>>>>>>>>> adding
> >>>>
> >>>>> constructors.
> >>>>>>>>>>>
> >>>>>>>>>>> Does anyone have a problem with me changing the above classes
> and
> >>>>>>>>>>> especially getting rid of the static initializers and singleton
> >>>>>>>>>>>
> >>>>>>>>>>> instances?
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>
> >
>

Re: Client Commands and SecurityService

[Kirk Lund <kl...@apache.org>]

My primary goal is to defer instantiating the Command instances until after
the Cache has initialized Security. I then have several options for
handling Security within the Commands that has much less performance impact
than it currently has. A major reconfiguration or lifecycle point, such as
closing and then recreating the Cache should be the only source of garbage
caused by throwing away the old instances and creating new ones.

Removal of singletons primarily involves moving the instantiation code and
the lifecycle of the class (that's a singleton) further up the layers of
Geode code so it becomes driven by Cache initialization rather than by
class loading. Conceptually, you would still only have one instance of
SecurityService for the Cache, but the code that's responsible for that
"singularity" are the collaborators that own and interact with the
instance. This allows us to replace the instance very easily between unit
tests or between lifecycles of the Cache (which is where a user would
reconfigure things) or even make future changes to support multiple
instances for some component types that are currently singletons.

What I'm planning is just a first pass at reducing our dependency on
singletons specifically to allow me to improve performance involving how
client Commands interact with Security (when security is enabled or
disabled).

On Mon, Apr 3, 2017 at 11:02 AM, Bruce Schuchardt <bs...@pivotal.io>
wrote:

> I'm not against refactoring the command classes.  They originated from the
> refactoring of a very large method that attempted to handle all client
> operations and, consequently, are currently stateless. You can't really
> hurt anything by creating multiple instances of them but please avoid
> creating unnecessary garbage.
>
>
>
> Le 4/3/2017 à 10:38 AM, Galen M O'Sullivan a écrit :
>
>> +1 to getting rid of singletons and non-constant use of static. Also to
>> code where the ownership semantics are clear.
>>
>> On Mon, Apr 3, 2017 at 10:30 AM, Jacob Barrett <jb...@pivotal.io>
>> wrote:
>>
>> +1 refactor
>>> On Mon, Apr 3, 2017 at 9:35 AM Michael William Dodge <md...@pivotal.io>
>>> wrote:
>>>
>>> +1 to modular and questioning non-constant use of static
>>>>
>>>> On 3 Apr, 2017, at 09:27, Anthony Baker <ab...@pivotal.io> wrote:
>>>>>
>>>>> Using singletons leads to very monolithic systems that are hard to test
>>>>>
>>>> and hard to change.  Instead we should prefer modular services like Udo
>>>> proposed.
>>>>
>>>>> I would go further and say that we should question any non-constant use
>>>>>
>>>> of “static”.
>>>>
>>>>> Anthony
>>>>>
>>>>> On Apr 3, 2017, at 9:01 AM, Udo Kohlmeyer <uk...@pivotal.io>
>>>>>>
>>>>> wrote:
>>>>
>>>>> Correct, that would be the definition.
>>>>>>
>>>>>> Yet, we find that our use of singletons within Geode is limiting to
>>>>>>
>>>>> say
>>>
>>>> that least. With the idea of wanting to be able to create/run multiple
>>>> cache instance within the same JVM (especially for testing) a singleton
>>>> will be problematic.
>>>>
>>>>> In addition to that, the alternative is not that hard to construct and
>>>>>>
>>>>> in many cases easier to manage.
>>>>
>>>>> --Udo
>>>>>>
>>>>>>
>>>>>> On 4/3/17 08:57, Jinmei Liao wrote:
>>>>>>
>>>>>>> Isn't "that instance is reused each invocation" my understanding of a
>>>>>>> "singleton"?
>>>>>>>
>>>>>>> On Mon, Apr 3, 2017 at 11:49 AM, Udo Kohlmeyer <
>>>>>>>
>>>>>> ukohlmeyer@pivotal.io>
>>>
>>>> wrote:
>>>>>>>
>>>>>>> -1 For using singletons
>>>>>>>>
>>>>>>>> Using a Factory pattern you can avoid having to create singletons in
>>>>>>>> addition to caching created commands to avoid the recreation of the
>>>>>>>> instance.
>>>>>>>>
>>>>>>>> The SSLConfigurationFactory is a simple example where you create an
>>>>>>>> instance when required. Once an instance is created, that instance
>>>>>>>>
>>>>>>> is
>>>
>>>> reused each invocation.
>>>>>>>>
>>>>>>>> --Udo
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 4/3/17 08:30, Jinmei Liao wrote:
>>>>>>>>
>>>>>>>> I think the client commands needs to be singleton instances even
>>>>>>>>>
>>>>>>>> after you
>>>>
>>>>> change the sequence of initialization. We don't want to have each
>>>>>>>>>
>>>>>>>> client
>>>>
>>>>> operation ends up creating a new command instance, right? That
>>>>>>>>>
>>>>>>>> would
>>>
>>>> be a
>>>>
>>>>> more performance drag.
>>>>>>>>>
>>>>>>>>> On Thu, Mar 30, 2017 at 2:14 PM, Kirk Lund <kl...@apache.org>
>>>>>>>>>
>>>>>>>> wrote:
>>>
>>>> PS: I'll be writing and using JMH benchmarks to drive these
>>>>>>>>>
>>>>>>>> changes.
>>>
>>>> I'll
>>>>
>>>>> also create new unit tests for each of these classes that don't
>>>>>>>>>>
>>>>>>>>> currently
>>>>
>>>>> have unit tests.
>>>>>>>>>>
>>>>>>>>>> On Thu, Mar 30, 2017 at 10:58 AM, Kirk Lund <kl...@apache.org>
>>>>>>>>>>
>>>>>>>>> wrote:
>>>>
>>>>> The client Commands now check with SecurityService even when
>>>>>>>>>>
>>>>>>>>> security is
>>>>
>>>>> not configured. This has introduced a negative performance
>>>>>>>>>>>
>>>>>>>>>> impact.
>>>
>>>> The best way to fix something like this is to tell the Command
>>>>>>>>>>>
>>>>>>>>>> instance
>>>>
>>>>> when it's being constructed that is does or does not need to
>>>>>>>>>>>
>>>>>>>>>> perform
>>>>
>>>>> security checks.
>>>>>>>>>>>
>>>>>>>>>>> Unfortunately, Commands are all implemented as singletons which
>>>>>>>>>>>
>>>>>>>>>> are very
>>>>
>>>>> eagerly instantiated during class loading of CommandInitializer
>>>>>>>>>>>
>>>>>>>>>> (who
>>>>
>>>>> thought that was a good idea?!).
>>>>>>>>>>>
>>>>>>>>>>> In order to fix this performance problem, I would need to get rid
>>>>>>>>>>>
>>>>>>>>>> of
>>>>
>>>>> these
>>>>>>>>>>
>>>>>>>>>> problematic static initializer blocks that so eagerly construct
>>>>>>>>>>>
>>>>>>>>>> the
>>>
>>>> Commands so that I can put off constructing them until AFTER the
>>>>>>>>>>>
>>>>>>>>>> Cache
>>>>
>>>>> is
>>>>>>>>>>> initializing and specifically AFTER the Cache has determined if
>>>>>>>>>>>
>>>>>>>>>> it
>>>
>>>> is
>>>>
>>>>> using
>>>>>>>>>>
>>>>>>>>>> security or not.
>>>>>>>>>>>
>>>>>>>>>>> This means I'm going to have to do some refactoring of
>>>>>>>>>>>
>>>>>>>>>>> CommandInitializer,
>>>>>>>>>>
>>>>>>>>>> the Command classes, ServerConnection, AcceptorImpl, etc.
>>>>>>>>>>>
>>>>>>>>>>> Any other approach is going to have such minimal impact on
>>>>>>>>>>>
>>>>>>>>>> performance
>>>>
>>>>> that I'm not even interested in doing less than this.
>>>>>>>>>>>
>>>>>>>>>>>  From a very high level, I would change the code so that the
>>>>>>>>>>> Cache
>>>>>>>>>>>
>>>>>>>>>> owns
>>>>
>>>>> the
>>>>>>>>>>
>>>>>>>>>> Server which owns the Command instances. In this way, the
>>>>>>>>>>>
>>>>>>>>>> configuring of
>>>>
>>>>> use of security can trickle down from Cache to each Command. I
>>>>>>>>>>>
>>>>>>>>>> would
>>>>
>>>>> primarily be altering static singletons, static initializers and
>>>>>>>>>>>
>>>>>>>>>> adding
>>>>
>>>>> constructors.
>>>>>>>>>>>
>>>>>>>>>>> Does anyone have a problem with me changing the above classes and
>>>>>>>>>>> especially getting rid of the static initializers and singleton
>>>>>>>>>>>
>>>>>>>>>>> instances?
>>>>>>>>>>
>>>>>>>>>>
>>>>
>

Re: Client Commands and SecurityService

[Bruce Schuchardt <bs...@pivotal.io>]

I'm not against refactoring the command classes.  They originated from 
the refactoring of a very large method that attempted to handle all 
client operations and, consequently, are currently stateless. You can't 
really hurt anything by creating multiple instances of them but please 
avoid creating unnecessary garbage.


Le 4/3/2017 � 10:38 AM, Galen M O'Sullivan a �crit :
> +1 to getting rid of singletons and non-constant use of static. Also to
> code where the ownership semantics are clear.
>
> On Mon, Apr 3, 2017 at 10:30 AM, Jacob Barrett <jb...@pivotal.io> wrote:
>
>> +1 refactor
>> On Mon, Apr 3, 2017 at 9:35 AM Michael William Dodge <md...@pivotal.io>
>> wrote:
>>
>>> +1 to modular and questioning non-constant use of static
>>>
>>>> On 3 Apr, 2017, at 09:27, Anthony Baker <ab...@pivotal.io> wrote:
>>>>
>>>> Using singletons leads to very monolithic systems that are hard to test
>>> and hard to change.  Instead we should prefer modular services like Udo
>>> proposed.
>>>> I would go further and say that we should question any non-constant use
>>> of \u201cstatic\u201d.
>>>> Anthony
>>>>
>>>>> On Apr 3, 2017, at 9:01 AM, Udo Kohlmeyer <uk...@pivotal.io>
>>> wrote:
>>>>> Correct, that would be the definition.
>>>>>
>>>>> Yet, we find that our use of singletons within Geode is limiting to
>> say
>>> that least. With the idea of wanting to be able to create/run multiple
>>> cache instance within the same JVM (especially for testing) a singleton
>>> will be problematic.
>>>>> In addition to that, the alternative is not that hard to construct and
>>> in many cases easier to manage.
>>>>> --Udo
>>>>>
>>>>>
>>>>> On 4/3/17 08:57, Jinmei Liao wrote:
>>>>>> Isn't "that instance is reused each invocation" my understanding of a
>>>>>> "singleton"?
>>>>>>
>>>>>> On Mon, Apr 3, 2017 at 11:49 AM, Udo Kohlmeyer <
>> ukohlmeyer@pivotal.io>
>>>>>> wrote:
>>>>>>
>>>>>>> -1 For using singletons
>>>>>>>
>>>>>>> Using a Factory pattern you can avoid having to create singletons in
>>>>>>> addition to caching created commands to avoid the recreation of the
>>>>>>> instance.
>>>>>>>
>>>>>>> The SSLConfigurationFactory is a simple example where you create an
>>>>>>> instance when required. Once an instance is created, that instance
>> is
>>>>>>> reused each invocation.
>>>>>>>
>>>>>>> --Udo
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 4/3/17 08:30, Jinmei Liao wrote:
>>>>>>>
>>>>>>>> I think the client commands needs to be singleton instances even
>>> after you
>>>>>>>> change the sequence of initialization. We don't want to have each
>>> client
>>>>>>>> operation ends up creating a new command instance, right? That
>> would
>>> be a
>>>>>>>> more performance drag.
>>>>>>>>
>>>>>>>> On Thu, Mar 30, 2017 at 2:14 PM, Kirk Lund <kl...@apache.org>
>> wrote:
>>>>>>>> PS: I'll be writing and using JMH benchmarks to drive these
>> changes.
>>> I'll
>>>>>>>>> also create new unit tests for each of these classes that don't
>>> currently
>>>>>>>>> have unit tests.
>>>>>>>>>
>>>>>>>>> On Thu, Mar 30, 2017 at 10:58 AM, Kirk Lund <kl...@apache.org>
>>> wrote:
>>>>>>>>> The client Commands now check with SecurityService even when
>>> security is
>>>>>>>>>> not configured. This has introduced a negative performance
>> impact.
>>>>>>>>>> The best way to fix something like this is to tell the Command
>>> instance
>>>>>>>>>> when it's being constructed that is does or does not need to
>>> perform
>>>>>>>>>> security checks.
>>>>>>>>>>
>>>>>>>>>> Unfortunately, Commands are all implemented as singletons which
>>> are very
>>>>>>>>>> eagerly instantiated during class loading of CommandInitializer
>>> (who
>>>>>>>>>> thought that was a good idea?!).
>>>>>>>>>>
>>>>>>>>>> In order to fix this performance problem, I would need to get rid
>>> of
>>>>>>>>> these
>>>>>>>>>
>>>>>>>>>> problematic static initializer blocks that so eagerly construct
>> the
>>>>>>>>>> Commands so that I can put off constructing them until AFTER the
>>> Cache
>>>>>>>>>> is
>>>>>>>>>> initializing and specifically AFTER the Cache has determined if
>> it
>>> is
>>>>>>>>> using
>>>>>>>>>
>>>>>>>>>> security or not.
>>>>>>>>>>
>>>>>>>>>> This means I'm going to have to do some refactoring of
>>>>>>>>>>
>>>>>>>>> CommandInitializer,
>>>>>>>>>
>>>>>>>>>> the Command classes, ServerConnection, AcceptorImpl, etc.
>>>>>>>>>>
>>>>>>>>>> Any other approach is going to have such minimal impact on
>>> performance
>>>>>>>>>> that I'm not even interested in doing less than this.
>>>>>>>>>>
>>>>>>>>>>  From a very high level, I would change the code so that the Cache
>>> owns
>>>>>>>>> the
>>>>>>>>>
>>>>>>>>>> Server which owns the Command instances. In this way, the
>>> configuring of
>>>>>>>>>> use of security can trickle down from Cache to each Command. I
>>> would
>>>>>>>>>> primarily be altering static singletons, static initializers and
>>> adding
>>>>>>>>>> constructors.
>>>>>>>>>>
>>>>>>>>>> Does anyone have a problem with me changing the above classes and
>>>>>>>>>> especially getting rid of the static initializers and singleton
>>>>>>>>>>
>>>>>>>>> instances?
>>>>>>>>>
>>>

Re: Client Commands and SecurityService

[Galen M O'Sullivan <go...@pivotal.io>]

+1 to getting rid of singletons and non-constant use of static. Also to
code where the ownership semantics are clear.

On Mon, Apr 3, 2017 at 10:30 AM, Jacob Barrett <jb...@pivotal.io> wrote:

> +1 refactor
> On Mon, Apr 3, 2017 at 9:35 AM Michael William Dodge <md...@pivotal.io>
> wrote:
>
> > +1 to modular and questioning non-constant use of static
> >
> > > On 3 Apr, 2017, at 09:27, Anthony Baker <ab...@pivotal.io> wrote:
> > >
> > > Using singletons leads to very monolithic systems that are hard to test
> > and hard to change.  Instead we should prefer modular services like Udo
> > proposed.
> > >
> > > I would go further and say that we should question any non-constant use
> > of “static”.
> > >
> > > Anthony
> > >
> > >> On Apr 3, 2017, at 9:01 AM, Udo Kohlmeyer <uk...@pivotal.io>
> > wrote:
> > >>
> > >> Correct, that would be the definition.
> > >>
> > >> Yet, we find that our use of singletons within Geode is limiting to
> say
> > that least. With the idea of wanting to be able to create/run multiple
> > cache instance within the same JVM (especially for testing) a singleton
> > will be problematic.
> > >>
> > >> In addition to that, the alternative is not that hard to construct and
> > in many cases easier to manage.
> > >>
> > >> --Udo
> > >>
> > >>
> > >> On 4/3/17 08:57, Jinmei Liao wrote:
> > >>> Isn't "that instance is reused each invocation" my understanding of a
> > >>> "singleton"?
> > >>>
> > >>> On Mon, Apr 3, 2017 at 11:49 AM, Udo Kohlmeyer <
> ukohlmeyer@pivotal.io>
> > >>> wrote:
> > >>>
> > >>>> -1 For using singletons
> > >>>>
> > >>>> Using a Factory pattern you can avoid having to create singletons in
> > >>>> addition to caching created commands to avoid the recreation of the
> > >>>> instance.
> > >>>>
> > >>>> The SSLConfigurationFactory is a simple example where you create an
> > >>>> instance when required. Once an instance is created, that instance
> is
> > >>>> reused each invocation.
> > >>>>
> > >>>> --Udo
> > >>>>
> > >>>>
> > >>>>
> > >>>> On 4/3/17 08:30, Jinmei Liao wrote:
> > >>>>
> > >>>>> I think the client commands needs to be singleton instances even
> > after you
> > >>>>> change the sequence of initialization. We don't want to have each
> > client
> > >>>>> operation ends up creating a new command instance, right? That
> would
> > be a
> > >>>>> more performance drag.
> > >>>>>
> > >>>>> On Thu, Mar 30, 2017 at 2:14 PM, Kirk Lund <kl...@apache.org>
> wrote:
> > >>>>>
> > >>>>> PS: I'll be writing and using JMH benchmarks to drive these
> changes.
> > I'll
> > >>>>>> also create new unit tests for each of these classes that don't
> > currently
> > >>>>>> have unit tests.
> > >>>>>>
> > >>>>>> On Thu, Mar 30, 2017 at 10:58 AM, Kirk Lund <kl...@apache.org>
> > wrote:
> > >>>>>>
> > >>>>>> The client Commands now check with SecurityService even when
> > security is
> > >>>>>>> not configured. This has introduced a negative performance
> impact.
> > >>>>>>>
> > >>>>>>> The best way to fix something like this is to tell the Command
> > instance
> > >>>>>>> when it's being constructed that is does or does not need to
> > perform
> > >>>>>>> security checks.
> > >>>>>>>
> > >>>>>>> Unfortunately, Commands are all implemented as singletons which
> > are very
> > >>>>>>> eagerly instantiated during class loading of CommandInitializer
> > (who
> > >>>>>>> thought that was a good idea?!).
> > >>>>>>>
> > >>>>>>> In order to fix this performance problem, I would need to get rid
> > of
> > >>>>>>>
> > >>>>>> these
> > >>>>>>
> > >>>>>>> problematic static initializer blocks that so eagerly construct
> the
> > >>>>>>> Commands so that I can put off constructing them until AFTER the
> > Cache
> > >>>>>>> is
> > >>>>>>> initializing and specifically AFTER the Cache has determined if
> it
> > is
> > >>>>>>>
> > >>>>>> using
> > >>>>>>
> > >>>>>>> security or not.
> > >>>>>>>
> > >>>>>>> This means I'm going to have to do some refactoring of
> > >>>>>>>
> > >>>>>> CommandInitializer,
> > >>>>>>
> > >>>>>>> the Command classes, ServerConnection, AcceptorImpl, etc.
> > >>>>>>>
> > >>>>>>> Any other approach is going to have such minimal impact on
> > performance
> > >>>>>>> that I'm not even interested in doing less than this.
> > >>>>>>>
> > >>>>>>> From a very high level, I would change the code so that the Cache
> > owns
> > >>>>>>>
> > >>>>>> the
> > >>>>>>
> > >>>>>>> Server which owns the Command instances. In this way, the
> > configuring of
> > >>>>>>> use of security can trickle down from Cache to each Command. I
> > would
> > >>>>>>> primarily be altering static singletons, static initializers and
> > adding
> > >>>>>>> constructors.
> > >>>>>>>
> > >>>>>>> Does anyone have a problem with me changing the above classes and
> > >>>>>>> especially getting rid of the static initializers and singleton
> > >>>>>>>
> > >>>>>> instances?
> > >>>>>>
> > >>>>>>>
> > >>>>>
> > >>>
> > >>
> > >
> >
> >
>

Re: Client Commands and SecurityService

[Jacob Barrett <jb...@pivotal.io>]

+1 refactor
On Mon, Apr 3, 2017 at 9:35 AM Michael William Dodge <md...@pivotal.io>
wrote:

> +1 to modular and questioning non-constant use of static
>
> > On 3 Apr, 2017, at 09:27, Anthony Baker <ab...@pivotal.io> wrote:
> >
> > Using singletons leads to very monolithic systems that are hard to test
> and hard to change.  Instead we should prefer modular services like Udo
> proposed.
> >
> > I would go further and say that we should question any non-constant use
> of “static”.
> >
> > Anthony
> >
> >> On Apr 3, 2017, at 9:01 AM, Udo Kohlmeyer <uk...@pivotal.io>
> wrote:
> >>
> >> Correct, that would be the definition.
> >>
> >> Yet, we find that our use of singletons within Geode is limiting to say
> that least. With the idea of wanting to be able to create/run multiple
> cache instance within the same JVM (especially for testing) a singleton
> will be problematic.
> >>
> >> In addition to that, the alternative is not that hard to construct and
> in many cases easier to manage.
> >>
> >> --Udo
> >>
> >>
> >> On 4/3/17 08:57, Jinmei Liao wrote:
> >>> Isn't "that instance is reused each invocation" my understanding of a
> >>> "singleton"?
> >>>
> >>> On Mon, Apr 3, 2017 at 11:49 AM, Udo Kohlmeyer <uk...@pivotal.io>
> >>> wrote:
> >>>
> >>>> -1 For using singletons
> >>>>
> >>>> Using a Factory pattern you can avoid having to create singletons in
> >>>> addition to caching created commands to avoid the recreation of the
> >>>> instance.
> >>>>
> >>>> The SSLConfigurationFactory is a simple example where you create an
> >>>> instance when required. Once an instance is created, that instance is
> >>>> reused each invocation.
> >>>>
> >>>> --Udo
> >>>>
> >>>>
> >>>>
> >>>> On 4/3/17 08:30, Jinmei Liao wrote:
> >>>>
> >>>>> I think the client commands needs to be singleton instances even
> after you
> >>>>> change the sequence of initialization. We don't want to have each
> client
> >>>>> operation ends up creating a new command instance, right? That would
> be a
> >>>>> more performance drag.
> >>>>>
> >>>>> On Thu, Mar 30, 2017 at 2:14 PM, Kirk Lund <kl...@apache.org> wrote:
> >>>>>
> >>>>> PS: I'll be writing and using JMH benchmarks to drive these changes.
> I'll
> >>>>>> also create new unit tests for each of these classes that don't
> currently
> >>>>>> have unit tests.
> >>>>>>
> >>>>>> On Thu, Mar 30, 2017 at 10:58 AM, Kirk Lund <kl...@apache.org>
> wrote:
> >>>>>>
> >>>>>> The client Commands now check with SecurityService even when
> security is
> >>>>>>> not configured. This has introduced a negative performance impact.
> >>>>>>>
> >>>>>>> The best way to fix something like this is to tell the Command
> instance
> >>>>>>> when it's being constructed that is does or does not need to
> perform
> >>>>>>> security checks.
> >>>>>>>
> >>>>>>> Unfortunately, Commands are all implemented as singletons which
> are very
> >>>>>>> eagerly instantiated during class loading of CommandInitializer
> (who
> >>>>>>> thought that was a good idea?!).
> >>>>>>>
> >>>>>>> In order to fix this performance problem, I would need to get rid
> of
> >>>>>>>
> >>>>>> these
> >>>>>>
> >>>>>>> problematic static initializer blocks that so eagerly construct the
> >>>>>>> Commands so that I can put off constructing them until AFTER the
> Cache
> >>>>>>> is
> >>>>>>> initializing and specifically AFTER the Cache has determined if it
> is
> >>>>>>>
> >>>>>> using
> >>>>>>
> >>>>>>> security or not.
> >>>>>>>
> >>>>>>> This means I'm going to have to do some refactoring of
> >>>>>>>
> >>>>>> CommandInitializer,
> >>>>>>
> >>>>>>> the Command classes, ServerConnection, AcceptorImpl, etc.
> >>>>>>>
> >>>>>>> Any other approach is going to have such minimal impact on
> performance
> >>>>>>> that I'm not even interested in doing less than this.
> >>>>>>>
> >>>>>>> From a very high level, I would change the code so that the Cache
> owns
> >>>>>>>
> >>>>>> the
> >>>>>>
> >>>>>>> Server which owns the Command instances. In this way, the
> configuring of
> >>>>>>> use of security can trickle down from Cache to each Command. I
> would
> >>>>>>> primarily be altering static singletons, static initializers and
> adding
> >>>>>>> constructors.
> >>>>>>>
> >>>>>>> Does anyone have a problem with me changing the above classes and
> >>>>>>> especially getting rid of the static initializers and singleton
> >>>>>>>
> >>>>>> instances?
> >>>>>>
> >>>>>>>
> >>>>>
> >>>
> >>
> >
>
>

Re: Client Commands and SecurityService

[Michael William Dodge <md...@pivotal.io>]

+1 to modular and questioning non-constant use of static

> On 3 Apr, 2017, at 09:27, Anthony Baker <ab...@pivotal.io> wrote:
> 
> Using singletons leads to very monolithic systems that are hard to test and hard to change.  Instead we should prefer modular services like Udo proposed.
> 
> I would go further and say that we should question any non-constant use of “static”.
> 
> Anthony
> 
>> On Apr 3, 2017, at 9:01 AM, Udo Kohlmeyer <uk...@pivotal.io> wrote:
>> 
>> Correct, that would be the definition.
>> 
>> Yet, we find that our use of singletons within Geode is limiting to say that least. With the idea of wanting to be able to create/run multiple cache instance within the same JVM (especially for testing) a singleton will be problematic.
>> 
>> In addition to that, the alternative is not that hard to construct and in many cases easier to manage.
>> 
>> --Udo
>> 
>> 
>> On 4/3/17 08:57, Jinmei Liao wrote:
>>> Isn't "that instance is reused each invocation" my understanding of a
>>> "singleton"?
>>> 
>>> On Mon, Apr 3, 2017 at 11:49 AM, Udo Kohlmeyer <uk...@pivotal.io>
>>> wrote:
>>> 
>>>> -1 For using singletons
>>>> 
>>>> Using a Factory pattern you can avoid having to create singletons in
>>>> addition to caching created commands to avoid the recreation of the
>>>> instance.
>>>> 
>>>> The SSLConfigurationFactory is a simple example where you create an
>>>> instance when required. Once an instance is created, that instance is
>>>> reused each invocation.
>>>> 
>>>> --Udo
>>>> 
>>>> 
>>>> 
>>>> On 4/3/17 08:30, Jinmei Liao wrote:
>>>> 
>>>>> I think the client commands needs to be singleton instances even after you
>>>>> change the sequence of initialization. We don't want to have each client
>>>>> operation ends up creating a new command instance, right? That would be a
>>>>> more performance drag.
>>>>> 
>>>>> On Thu, Mar 30, 2017 at 2:14 PM, Kirk Lund <kl...@apache.org> wrote:
>>>>> 
>>>>> PS: I'll be writing and using JMH benchmarks to drive these changes. I'll
>>>>>> also create new unit tests for each of these classes that don't currently
>>>>>> have unit tests.
>>>>>> 
>>>>>> On Thu, Mar 30, 2017 at 10:58 AM, Kirk Lund <kl...@apache.org> wrote:
>>>>>> 
>>>>>> The client Commands now check with SecurityService even when security is
>>>>>>> not configured. This has introduced a negative performance impact.
>>>>>>> 
>>>>>>> The best way to fix something like this is to tell the Command instance
>>>>>>> when it's being constructed that is does or does not need to perform
>>>>>>> security checks.
>>>>>>> 
>>>>>>> Unfortunately, Commands are all implemented as singletons which are very
>>>>>>> eagerly instantiated during class loading of CommandInitializer (who
>>>>>>> thought that was a good idea?!).
>>>>>>> 
>>>>>>> In order to fix this performance problem, I would need to get rid of
>>>>>>> 
>>>>>> these
>>>>>> 
>>>>>>> problematic static initializer blocks that so eagerly construct the
>>>>>>> Commands so that I can put off constructing them until AFTER the Cache
>>>>>>> is
>>>>>>> initializing and specifically AFTER the Cache has determined if it is
>>>>>>> 
>>>>>> using
>>>>>> 
>>>>>>> security or not.
>>>>>>> 
>>>>>>> This means I'm going to have to do some refactoring of
>>>>>>> 
>>>>>> CommandInitializer,
>>>>>> 
>>>>>>> the Command classes, ServerConnection, AcceptorImpl, etc.
>>>>>>> 
>>>>>>> Any other approach is going to have such minimal impact on performance
>>>>>>> that I'm not even interested in doing less than this.
>>>>>>> 
>>>>>>> From a very high level, I would change the code so that the Cache owns
>>>>>>> 
>>>>>> the
>>>>>> 
>>>>>>> Server which owns the Command instances. In this way, the configuring of
>>>>>>> use of security can trickle down from Cache to each Command. I would
>>>>>>> primarily be altering static singletons, static initializers and adding
>>>>>>> constructors.
>>>>>>> 
>>>>>>> Does anyone have a problem with me changing the above classes and
>>>>>>> especially getting rid of the static initializers and singleton
>>>>>>> 
>>>>>> instances?
>>>>>> 
>>>>>>> 
>>>>> 
>>> 
>> 
> 

Re: Client Commands and SecurityService

[Anthony Baker <ab...@pivotal.io>]

Using singletons leads to very monolithic systems that are hard to test and hard to change.  Instead we should prefer modular services like Udo proposed.

I would go further and say that we should question any non-constant use of “static”.

Anthony

> On Apr 3, 2017, at 9:01 AM, Udo Kohlmeyer <uk...@pivotal.io> wrote:
> 
> Correct, that would be the definition.
> 
> Yet, we find that our use of singletons within Geode is limiting to say that least. With the idea of wanting to be able to create/run multiple cache instance within the same JVM (especially for testing) a singleton will be problematic.
> 
> In addition to that, the alternative is not that hard to construct and in many cases easier to manage.
> 
> --Udo
> 
> 
> On 4/3/17 08:57, Jinmei Liao wrote:
>> Isn't "that instance is reused each invocation" my understanding of a
>> "singleton"?
>> 
>> On Mon, Apr 3, 2017 at 11:49 AM, Udo Kohlmeyer <uk...@pivotal.io>
>> wrote:
>> 
>>> -1 For using singletons
>>> 
>>> Using a Factory pattern you can avoid having to create singletons in
>>> addition to caching created commands to avoid the recreation of the
>>> instance.
>>> 
>>> The SSLConfigurationFactory is a simple example where you create an
>>> instance when required. Once an instance is created, that instance is
>>> reused each invocation.
>>> 
>>> --Udo
>>> 
>>> 
>>> 
>>> On 4/3/17 08:30, Jinmei Liao wrote:
>>> 
>>>> I think the client commands needs to be singleton instances even after you
>>>> change the sequence of initialization. We don't want to have each client
>>>> operation ends up creating a new command instance, right? That would be a
>>>> more performance drag.
>>>> 
>>>> On Thu, Mar 30, 2017 at 2:14 PM, Kirk Lund <kl...@apache.org> wrote:
>>>> 
>>>> PS: I'll be writing and using JMH benchmarks to drive these changes. I'll
>>>>> also create new unit tests for each of these classes that don't currently
>>>>> have unit tests.
>>>>> 
>>>>> On Thu, Mar 30, 2017 at 10:58 AM, Kirk Lund <kl...@apache.org> wrote:
>>>>> 
>>>>> The client Commands now check with SecurityService even when security is
>>>>>> not configured. This has introduced a negative performance impact.
>>>>>> 
>>>>>> The best way to fix something like this is to tell the Command instance
>>>>>> when it's being constructed that is does or does not need to perform
>>>>>> security checks.
>>>>>> 
>>>>>> Unfortunately, Commands are all implemented as singletons which are very
>>>>>> eagerly instantiated during class loading of CommandInitializer (who
>>>>>> thought that was a good idea?!).
>>>>>> 
>>>>>> In order to fix this performance problem, I would need to get rid of
>>>>>> 
>>>>> these
>>>>> 
>>>>>> problematic static initializer blocks that so eagerly construct the
>>>>>> Commands so that I can put off constructing them until AFTER the Cache
>>>>>> is
>>>>>> initializing and specifically AFTER the Cache has determined if it is
>>>>>> 
>>>>> using
>>>>> 
>>>>>> security or not.
>>>>>> 
>>>>>> This means I'm going to have to do some refactoring of
>>>>>> 
>>>>> CommandInitializer,
>>>>> 
>>>>>> the Command classes, ServerConnection, AcceptorImpl, etc.
>>>>>> 
>>>>>> Any other approach is going to have such minimal impact on performance
>>>>>> that I'm not even interested in doing less than this.
>>>>>> 
>>>>>>  From a very high level, I would change the code so that the Cache owns
>>>>>> 
>>>>> the
>>>>> 
>>>>>> Server which owns the Command instances. In this way, the configuring of
>>>>>> use of security can trickle down from Cache to each Command. I would
>>>>>> primarily be altering static singletons, static initializers and adding
>>>>>> constructors.
>>>>>> 
>>>>>> Does anyone have a problem with me changing the above classes and
>>>>>> especially getting rid of the static initializers and singleton
>>>>>> 
>>>>> instances?
>>>>> 
>>>>>> 
>>>> 
>> 
> 

Re: Client Commands and SecurityService

[Udo Kohlmeyer <uk...@pivotal.io>]

Correct, that would be the definition.

Yet, we find that our use of singletons within Geode is limiting to say 
that least. With the idea of wanting to be able to create/run multiple 
cache instance within the same JVM (especially for testing) a singleton 
will be problematic.

In addition to that, the alternative is not that hard to construct and 
in many cases easier to manage.

--Udo


On 4/3/17 08:57, Jinmei Liao wrote:
> Isn't "that instance is reused each invocation" my understanding of a
> "singleton"?
>
> On Mon, Apr 3, 2017 at 11:49 AM, Udo Kohlmeyer <uk...@pivotal.io>
> wrote:
>
>> -1 For using singletons
>>
>> Using a Factory pattern you can avoid having to create singletons in
>> addition to caching created commands to avoid the recreation of the
>> instance.
>>
>> The SSLConfigurationFactory is a simple example where you create an
>> instance when required. Once an instance is created, that instance is
>> reused each invocation.
>>
>> --Udo
>>
>>
>>
>> On 4/3/17 08:30, Jinmei Liao wrote:
>>
>>> I think the client commands needs to be singleton instances even after you
>>> change the sequence of initialization. We don't want to have each client
>>> operation ends up creating a new command instance, right? That would be a
>>> more performance drag.
>>>
>>> On Thu, Mar 30, 2017 at 2:14 PM, Kirk Lund <kl...@apache.org> wrote:
>>>
>>> PS: I'll be writing and using JMH benchmarks to drive these changes. I'll
>>>> also create new unit tests for each of these classes that don't currently
>>>> have unit tests.
>>>>
>>>> On Thu, Mar 30, 2017 at 10:58 AM, Kirk Lund <kl...@apache.org> wrote:
>>>>
>>>> The client Commands now check with SecurityService even when security is
>>>>> not configured. This has introduced a negative performance impact.
>>>>>
>>>>> The best way to fix something like this is to tell the Command instance
>>>>> when it's being constructed that is does or does not need to perform
>>>>> security checks.
>>>>>
>>>>> Unfortunately, Commands are all implemented as singletons which are very
>>>>> eagerly instantiated during class loading of CommandInitializer (who
>>>>> thought that was a good idea?!).
>>>>>
>>>>> In order to fix this performance problem, I would need to get rid of
>>>>>
>>>> these
>>>>
>>>>> problematic static initializer blocks that so eagerly construct the
>>>>> Commands so that I can put off constructing them until AFTER the Cache
>>>>> is
>>>>> initializing and specifically AFTER the Cache has determined if it is
>>>>>
>>>> using
>>>>
>>>>> security or not.
>>>>>
>>>>> This means I'm going to have to do some refactoring of
>>>>>
>>>> CommandInitializer,
>>>>
>>>>> the Command classes, ServerConnection, AcceptorImpl, etc.
>>>>>
>>>>> Any other approach is going to have such minimal impact on performance
>>>>> that I'm not even interested in doing less than this.
>>>>>
>>>>>   From a very high level, I would change the code so that the Cache owns
>>>>>
>>>> the
>>>>
>>>>> Server which owns the Command instances. In this way, the configuring of
>>>>> use of security can trickle down from Cache to each Command. I would
>>>>> primarily be altering static singletons, static initializers and adding
>>>>> constructors.
>>>>>
>>>>> Does anyone have a problem with me changing the above classes and
>>>>> especially getting rid of the static initializers and singleton
>>>>>
>>>> instances?
>>>>
>>>>>
>>>
>

Re: Client Commands and SecurityService

[Jinmei Liao <ji...@pivotal.io>]

Isn't "that instance is reused each invocation" my understanding of a
"singleton"?

On Mon, Apr 3, 2017 at 11:49 AM, Udo Kohlmeyer <uk...@pivotal.io>
wrote:

> -1 For using singletons
>
> Using a Factory pattern you can avoid having to create singletons in
> addition to caching created commands to avoid the recreation of the
> instance.
>
> The SSLConfigurationFactory is a simple example where you create an
> instance when required. Once an instance is created, that instance is
> reused each invocation.
>
> --Udo
>
>
>
> On 4/3/17 08:30, Jinmei Liao wrote:
>
>> I think the client commands needs to be singleton instances even after you
>> change the sequence of initialization. We don't want to have each client
>> operation ends up creating a new command instance, right? That would be a
>> more performance drag.
>>
>> On Thu, Mar 30, 2017 at 2:14 PM, Kirk Lund <kl...@apache.org> wrote:
>>
>> PS: I'll be writing and using JMH benchmarks to drive these changes. I'll
>>> also create new unit tests for each of these classes that don't currently
>>> have unit tests.
>>>
>>> On Thu, Mar 30, 2017 at 10:58 AM, Kirk Lund <kl...@apache.org> wrote:
>>>
>>> The client Commands now check with SecurityService even when security is
>>>> not configured. This has introduced a negative performance impact.
>>>>
>>>> The best way to fix something like this is to tell the Command instance
>>>> when it's being constructed that is does or does not need to perform
>>>> security checks.
>>>>
>>>> Unfortunately, Commands are all implemented as singletons which are very
>>>> eagerly instantiated during class loading of CommandInitializer (who
>>>> thought that was a good idea?!).
>>>>
>>>> In order to fix this performance problem, I would need to get rid of
>>>>
>>> these
>>>
>>>> problematic static initializer blocks that so eagerly construct the
>>>> Commands so that I can put off constructing them until AFTER the Cache
>>>> is
>>>> initializing and specifically AFTER the Cache has determined if it is
>>>>
>>> using
>>>
>>>> security or not.
>>>>
>>>> This means I'm going to have to do some refactoring of
>>>>
>>> CommandInitializer,
>>>
>>>> the Command classes, ServerConnection, AcceptorImpl, etc.
>>>>
>>>> Any other approach is going to have such minimal impact on performance
>>>> that I'm not even interested in doing less than this.
>>>>
>>>>  From a very high level, I would change the code so that the Cache owns
>>>>
>>> the
>>>
>>>> Server which owns the Command instances. In this way, the configuring of
>>>> use of security can trickle down from Cache to each Command. I would
>>>> primarily be altering static singletons, static initializers and adding
>>>> constructors.
>>>>
>>>> Does anyone have a problem with me changing the above classes and
>>>> especially getting rid of the static initializers and singleton
>>>>
>>> instances?
>>>
>>>>
>>>>
>>
>>
>


-- 
Cheers

Jinmei

Re: Client Commands and SecurityService

[Udo Kohlmeyer <uk...@pivotal.io>]

-1 For using singletons

Using a Factory pattern you can avoid having to create singletons in 
addition to caching created commands to avoid the recreation of the 
instance.

The SSLConfigurationFactory is a simple example where you create an 
instance when required. Once an instance is created, that instance is 
reused each invocation.

--Udo


On 4/3/17 08:30, Jinmei Liao wrote:
> I think the client commands needs to be singleton instances even after you
> change the sequence of initialization. We don't want to have each client
> operation ends up creating a new command instance, right? That would be a
> more performance drag.
>
> On Thu, Mar 30, 2017 at 2:14 PM, Kirk Lund <kl...@apache.org> wrote:
>
>> PS: I'll be writing and using JMH benchmarks to drive these changes. I'll
>> also create new unit tests for each of these classes that don't currently
>> have unit tests.
>>
>> On Thu, Mar 30, 2017 at 10:58 AM, Kirk Lund <kl...@apache.org> wrote:
>>
>>> The client Commands now check with SecurityService even when security is
>>> not configured. This has introduced a negative performance impact.
>>>
>>> The best way to fix something like this is to tell the Command instance
>>> when it's being constructed that is does or does not need to perform
>>> security checks.
>>>
>>> Unfortunately, Commands are all implemented as singletons which are very
>>> eagerly instantiated during class loading of CommandInitializer (who
>>> thought that was a good idea?!).
>>>
>>> In order to fix this performance problem, I would need to get rid of
>> these
>>> problematic static initializer blocks that so eagerly construct the
>>> Commands so that I can put off constructing them until AFTER the Cache is
>>> initializing and specifically AFTER the Cache has determined if it is
>> using
>>> security or not.
>>>
>>> This means I'm going to have to do some refactoring of
>> CommandInitializer,
>>> the Command classes, ServerConnection, AcceptorImpl, etc.
>>>
>>> Any other approach is going to have such minimal impact on performance
>>> that I'm not even interested in doing less than this.
>>>
>>>  From a very high level, I would change the code so that the Cache owns
>> the
>>> Server which owns the Command instances. In this way, the configuring of
>>> use of security can trickle down from Cache to each Command. I would
>>> primarily be altering static singletons, static initializers and adding
>>> constructors.
>>>
>>> Does anyone have a problem with me changing the above classes and
>>> especially getting rid of the static initializers and singleton
>> instances?
>>>
>
>

Re: Client Commands and SecurityService

[Jinmei Liao <ji...@pivotal.io>]

I think the client commands needs to be singleton instances even after you
change the sequence of initialization. We don't want to have each client
operation ends up creating a new command instance, right? That would be a
more performance drag.

On Thu, Mar 30, 2017 at 2:14 PM, Kirk Lund <kl...@apache.org> wrote:

> PS: I'll be writing and using JMH benchmarks to drive these changes. I'll
> also create new unit tests for each of these classes that don't currently
> have unit tests.
>
> On Thu, Mar 30, 2017 at 10:58 AM, Kirk Lund <kl...@apache.org> wrote:
>
> > The client Commands now check with SecurityService even when security is
> > not configured. This has introduced a negative performance impact.
> >
> > The best way to fix something like this is to tell the Command instance
> > when it's being constructed that is does or does not need to perform
> > security checks.
> >
> > Unfortunately, Commands are all implemented as singletons which are very
> > eagerly instantiated during class loading of CommandInitializer (who
> > thought that was a good idea?!).
> >
> > In order to fix this performance problem, I would need to get rid of
> these
> > problematic static initializer blocks that so eagerly construct the
> > Commands so that I can put off constructing them until AFTER the Cache is
> > initializing and specifically AFTER the Cache has determined if it is
> using
> > security or not.
> >
> > This means I'm going to have to do some refactoring of
> CommandInitializer,
> > the Command classes, ServerConnection, AcceptorImpl, etc.
> >
> > Any other approach is going to have such minimal impact on performance
> > that I'm not even interested in doing less than this.
> >
> > From a very high level, I would change the code so that the Cache owns
> the
> > Server which owns the Command instances. In this way, the configuring of
> > use of security can trickle down from Cache to each Command. I would
> > primarily be altering static singletons, static initializers and adding
> > constructors.
> >
> > Does anyone have a problem with me changing the above classes and
> > especially getting rid of the static initializers and singleton
> instances?
> >
> >
>



-- 
Cheers

Jinmei

Re: Client Commands and SecurityService

[Kirk Lund <kl...@apache.org>]

PS: I'll be writing and using JMH benchmarks to drive these changes. I'll
also create new unit tests for each of these classes that don't currently
have unit tests.

On Thu, Mar 30, 2017 at 10:58 AM, Kirk Lund <kl...@apache.org> wrote:

> The client Commands now check with SecurityService even when security is
> not configured. This has introduced a negative performance impact.
>
> The best way to fix something like this is to tell the Command instance
> when it's being constructed that is does or does not need to perform
> security checks.
>
> Unfortunately, Commands are all implemented as singletons which are very
> eagerly instantiated during class loading of CommandInitializer (who
> thought that was a good idea?!).
>
> In order to fix this performance problem, I would need to get rid of these
> problematic static initializer blocks that so eagerly construct the
> Commands so that I can put off constructing them until AFTER the Cache is
> initializing and specifically AFTER the Cache has determined if it is using
> security or not.
>
> This means I'm going to have to do some refactoring of CommandInitializer,
> the Command classes, ServerConnection, AcceptorImpl, etc.
>
> Any other approach is going to have such minimal impact on performance
> that I'm not even interested in doing less than this.
>
> From a very high level, I would change the code so that the Cache owns the
> Server which owns the Command instances. In this way, the configuring of
> use of security can trickle down from Cache to each Command. I would
> primarily be altering static singletons, static initializers and adding
> constructors.
>
> Does anyone have a problem with me changing the above classes and
> especially getting rid of the static initializers and singleton instances?
>
>