geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jacob Barrett <jbarr...@pivotal.io>
Subject Re: Client Commands and SecurityService
Date Fri, 28 Apr 2017 01:17:37 GMT
+1

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


-Jake

On Mon, Apr 3, 2017 at 1:30 PM Kirk Lund <klund@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 <bschuchardt@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 <jbarrett@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 <abaker@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 <ukohlmeyer@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 <klund@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
<klund@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?
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message