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