[
https://issues.apache.org/jira/browse/GEODE-7531?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16988282#comment-16988282
]
John Blum commented on GEODE-7531:
----------------------------------
Off topic... Apache Geode's notion of internal APIs is broken. Simply
declaring an interface or class (or other type) in an "internal" package is not
using the appropriate (Java) language constructs to properly protect an API.
Additionally, Apache Geode's public API is broken in many ways (bloated and
unintuitive interfaces like Region, functionality in "internal"
classes/interfaces that should be in an "external" interface/API, really poor
method signatures, etc).
Providing a "mock" {{PoolManager}} is a not an option given the interface only
provides static methods. _PowerMock_ is not a good substitute and quite the
wrong way to mock things, so I will not go down this path.
Whatever the outcome is here, there is no justification for exposing the
implementation details of 1 class to another class (e.g. the implementation of
whether a Pool is in use or not using an attach count), even if the classes
involved are somehow related (by package, library, ...), as {{PoolImpl}} is to
{{PoolManagerImpl}}. Even between classes in the same package/library,
developers should be careful to "_program to interfaces_" for the exact reasons
I mention above, especially when the API does expose certain publicly
accessible interfaces and methods ("internal" or otherwise).
Regarding...
> _I don't think it was ever intended to allow the install of a Pool into the
> PoolManager from some external source._
Why not? This is very useful from a framework or tooling perspective. In some
cases, it might even be useful for some types of applications, although I do
imagine less common. See [GEODE-7532].
> _While singletons do suck_
_Singletons_ don't suck (necessarily) and are a really useful pattern in the
right context, such as for shared, stateful or non-stateful resources and/or
beans. However, in this case, the {{PoolManager}} is stateful and uses statics
accessors to access shared state. That sucks!
Additionally, it is naive to assume all implementations, even in Geode's case
will always be a {{PoolImpl}}. What if Geode provides a different
implementation of Pool in the future, for another purpose? For most frameworks
and systems like Apache Geode, it is very wise to think terms of the Open/Close
principle (Open for extension, Closed for modification), which only works when
your types are 1) modeled correctly and 2) very extensible/flexible.
{{PoolManager}} is neither.
In the worse case scenario, you would have spaghetti code (and Geode does (!)
in some case; argh) like this scrambled through out the Geode codebase....
{code:java}
if (pool instanceof PoolImpl) {
((PoolImpl) pool).invokePoolImplSpecificMethod();
}
{code}
As you can imagine, that gets ugly real fast and simultaneously increases th
_Cyclomatic Complexity_ of the codebase, which is why it is rather better to
"_program to interfaces_" (and employ _polymorphism_) in the first place.
The fact of the matter is, {{PoolManager}} is a useful class despite being
designed and implemented poorly. The {{PoolManager}} is less of a factory and
more of a resource manager, for "managing" resources in a uniform fashion.
FWIW, the implementation of the changes called out by this ticket is plain
wrong, right down to the misleading {{IllegalStateException}} message.
> PoolManagerImpl.unregister(:Pool) naively assumes all Pool object instances
> are PoolImpls
> -----------------------------------------------------------------------------------------
>
> Key: GEODE-7531
> URL: https://issues.apache.org/jira/browse/GEODE-7531
> Project: Geode
> Issue Type: Bug
> Components: client/server
> Affects Versions: 1.10.0
> Environment: Apache Geode based applications on the JVM.
> Reporter: John Blum
> Priority: Blocker
>
> A recent
> [change|https://github.com/apache/geode/blob/rel/v1.10.0/geode-core/src/main/java/org/apache/geode/internal/cache/PoolManagerImpl.java#L169-L174]
> to the {{o.a.g.internal.cache.PoolManagerImpl}} class expects all
> {{o.a.g.cache.client.Pools}} registered with the
> {{o.a.g.cache.client.PoolManager}} to be
> {{o.a.g.cache.client.internal.PoolImp}} objects.
> This is certainly not going to be the case for Unit Tests that properly
> "mock" 1 or more {{Pool}} instances and additionally needs to register the
> mock {{Pool}} instances with the {{PoolManager}}. While the later may not be
> as common for application code, it is more certainly common, and in some
> cases necessary, for framework or tooling code.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
