You mentioned the metrics service - I think that is a good pattern to follow.
For metrics the user passes configuration (The user's MeterRegistry) into the
CacheFactory, and then the MetricsService hangs off the cache. Places in the
code that need metrics fetch the MeterRegistry from the cache.
I don't really see this ServiceRegistryInstance as something that we would want
to encourage any developers to use, since it is a singleton. I can see you
might need a singleton for the ClassLoaderService because it's too hard to
plumb your service into all the places you need it (I can understand that!) and
ClassPathLoader is already a singleton. But we don't really want anything else
to go in there, because of all the reasons we hate singletons.
For that reason, maybe it would be better just not to have this thing. Instead
have a ClassPathServiceSingleton that is clearly a wart we should fix, or hang
the ClassPathService off the cache, since we are already trying to find and
eliminate places people are calling Cache.getInstance. That way you are not
creating a tempting place for developers to stick more services where we really
don't want them to.
-Dan
PS - I do like the idea of introducing a ServiceRegistry or something similar
(Context, ?). It just should be scoped to a Cache or some shorter lifecycle.
And it should be immutable (I believe spring's ApplicationContext might be?),
so no adding and removing services from an instance of the ServiceRegistry or
Context or whatever.
________________________________
From: Udo Kohlmeyer <u...@vmware.com>
Sent: Tuesday, October 20, 2020 3:01 PM
To: dev@geode.apache.org <dev@geode.apache.org>
Subject: Re: [DISCUSS] ServiceRegistry RFC
Jens, that is a great thought. Lifecycle management would be amazing and I
think we can definitely think about that. Right now, my thought of the services
that are complimentary to the Cache. i.e the Cache uses them, but they
themselves are not dependent on them.
I still see things like HttpService/GeodeRedisService/LuceneService to be
modules that are dependent on the cache. Whereas the services that I’m looking
at registering in the ServiceRegistry are services that will be used by the
Cache and have no direct dependency on the Cache or it’s lifecycle.
e.g MetricsService can life outside of the cache, as there is no dependency on
the cache to log a metric. The same would be true for the ClassLoaderService.
As the reconnecting of the cache would not have an effect on the classes loaded.
But I like the idea of a lifecycle, but I think that discussion is better left
for the broader discussion on lifecycle management of the Cache.
Hope this makes sense.
--Udo
From: Jens Deppe <jde...@vmware.com>
Date: Wednesday, October 21, 2020 at 8:46 AM
To: dev@geode.apache.org <dev@geode.apache.org>
Subject: Re: [DISCUSS] ServiceRegistry RFC
One aspect this conversation has brought to mind is whether these services will
require some kind of lifecycle management? If any of them require an instance
of a Cache (and perhaps other components) they will need to be aware of the
cache restarting (happens during reconnect).
Is that going to be a problem?
--Jens
On 10/20/20, 1:12 PM, "Udo Kohlmeyer" <u...@vmware.com> wrote:
Hi there Dan,
We (Patrick and I) are in violent agreement about adding new singletons to
the product. This singleton is merely there to avoid the complete utter
re-write of the system to wire in said ServiceRegistry into the cache.
Yes, we are merely replacing 1 singleton with another, but we see it as a
step in the right direction that we are replacing the usage with a generic
service based implementation, that would allow for the simple “swap-in/out” of
class loading (GEODE-8067).
Our inspiration for this ServiceRegistry is from Spring’s
ApplicationContext, without all of the cool DI features that Spring would
bring. We just needed a single location where one could add services and lookup
them up.
We agree, singletons are bad. But hanging everything off InteralCache
without a better designed lifecycle for module/component creation would make no
sense. THAT is a design that we need to look at to better structure/separate
modules/components.
Whilst doing GEODE-8466, and the replacing of ClassPathLoader with an
instance-based ClassLoaderService, we found MANY static methods/functions that
used the singleton ClassPathLoader, without ANY possibility to be able to wire
in a ServiceRegistry instance. In addition, the current singleton is merely
reducing the number of files that we need to alter to get this added. We had
(from initial GEODE-8466) implementations, learned that passing in a
ClassLoaderService instance into each Class/Method that required it, meant we
touched 330+ classes. In addition those changes were BEFORE we attempted to
replace the static usages of ClassPathLoader with ClassLoaderService.
I imagine that the casuality list of Classes touched would be much higher
than 330 if we attempted the same approach with ClassPathLoader. In addition to
the crazy effort we’d have to go through to replace the static method/code
block usages of ClassPathLoader with instance references to ClassLoaderService.
--Udo
From: Dan Smith <dasm...@vmware.com>
Date: Wednesday, October 21, 2020 at 5:14 AM
To: dev@geode.apache.org <dev@geode.apache.org>
Subject: Re: [DISCUSS] ServiceRegistry RFC
It might be better to hang things off InternalCache rather than create a
new singleton. The cache is already a singleton, but we've been working on
removing that by plumbing the cache everywhere. The cache is effectively our
context object, and once we've finished removing calls to Cache.getInstance we
will have a context object that is not a singleton.
As Jens mentioned, the cache already has the concept of CacheServices that
can be retrieved using methods similar to what you listened in this RFC - eg
InternalCache.getService(Class clazz).
Can we reuse/generalize/extend this existing service concept for your case?
-Dan
________________________________
From: Jens Deppe <jde...@vmware.com>
Sent: Tuesday, October 20, 2020 5:33 AM
To: dev@geode.apache.org <dev@geode.apache.org>
Subject: Re: [DISCUSS] ServiceRegistry RFC
Hi Udo,
Is the intention of this to replace (and extend) what we're doing with the
traditional service loading mechanism today? i.e. how we're loading everything
that extends CacheService (for example HttpService, LuceneService,
GeodeRedisService, etc.).
Thanks
--jens
On 10/15/20, 7:25 PM, "Udo Kohlmeyer" <u...@vmware.com> wrote:
Hi there Apache Geode Devs.
Please find attached an RFC for the introduction of a ServiceRegistry
into Apache Geode.
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcwiki.apache.org%2Fconfluence%2Fdisplay%2FGEODE%2FServiceRegistry&data=04%7C01%7Cdasmith%40vmware.com%7Cfd01791904894aa6b3bf08d87543c0c2%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637388281158007225%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=NXRaew0%2F6fOPLK96k1yMx7ukkD57UOXsPdW9EYs5SAY%3D&reserved=0
Please add all comments to the RFC to this email for tracking and
discussion.
--Udo
