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