+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 <jbarr...@pivotal.io> wrote: > +1 refactor > On Mon, Apr 3, 2017 at 9:35 AM Michael William Dodge <mdo...@pivotal.io> > wrote: > > > +1 to modular and questioning non-constant use of static > > > > > On 3 Apr, 2017, at 09:27, Anthony Baker <aba...@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 <ukohlme...@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 < > ukohlme...@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? > > >>>>>> > > >>>>>>> > > >>>>> > > >>> > > >> > > > > > > > >
