[ https://issues.apache.org/jira/browse/GEODE-7531?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16988096#comment-16988096 ]
John Blum edited comment on GEODE-7531 at 12/4/19 7:08 PM: ----------------------------------------------------------- This [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] (from this [commit|https://github.com/apache/geode/commit/51dd01cf309f1fccea9fe41e08fab42ed9759e52#diff-8d7c80296713855c7f737671ad251e9a]) would have been better designed and implemented with something like the following: 1) On the {{Pool}} interface, declare a {{isInUse():boolean}} method. {code:java} interface Pool { boolean isInUse(); ... } {code} 2) Then, in the {{PoolManagerImpl}} class, rather than exposing the internal implementation of {{PoolImpl}} for "how" a {{Pool's}} usage is tracked (i.e. with an {{attacheCount}} property and an {{AtomicInteger}}, as seen [here|https://github.com/apache/geode/blob/rel/v1.10.0/geode-core/src/main/java/org/apache/geode/cache/client/internal/PoolImpl.java#L1113-L1141] and [here|https://github.com/apache/geode/blob/rel/v1.10.0/geode-core/src/main/java/org/apache/geode/cache/client/internal/PoolImpl.java#L1102-L1108]; BTW, this *NOT* {{since}} *GemFire 5.7* and no references to GemFire should be in the Geode codebase in the first place!), the {{PoolManagerImpl}} can strictly adhere to the interface (this is what is referred to as "_programming to interfaces_", the "contract", and not the implementation). {code:java} if (pool.isInUse()) { throw new IllegalStateException("Pool could not be destroyed because it is still in use!"); } {code} Alternatively, and for convenience as well as "_encapsulation_", a method could be provided on the {{Pool}} interface to assert that a {{Pool}} is not in use. For instance: {code:java} interface Pool { boolean isInUse(); default void assertNotInUse() { throw new IllegalStateException("Pool is in use!"); } } {code} The {{assertNotInUse()}} method could be *overloaded* to take a String message as well. {code:java} interface Pool { boolean isInUse(); default void assertNotInUse() { assertNotInUse("Pool is in use!"); } default void assertNotInUse(String message) { throw new IllegalStateException(message); } } {code} The advantage of having this method on the {{Pool}} interface is that it allows the type of {{Exception}} thrown to be consistently applied everywhere in Geode that the usage of the {{Pool}} matters before acting (such as in the {{PoolManager}} when "unregistering" the {{Pool}}, I guess). Additionally, it is assumed that a {{Pool}} would contain all the information necessary to craft an informative and useful {{Exception}} message. I'd also argue that the original message stating "how many" Regions is useless information actually: {code:java} throw new IllegalStateException(String.format("Pool could not be destroyed because it is still in use by %s regions", attachCount)); {code} For debugging purposes, I'd rather know "which" Regions (by "name") were still use by the `Pool` in question. Additionally, the Exception message is actually misleading since in the {{PoolManagerImpl.unregister(:Pool)}} method, the {{Pool}} is not being "destroyed", it is simply being "unregistered" (i.e. "unmanaged"). Furthermore, it is NOT only Regions that use a {{Pool}}. It is also CQs and Functions, etc. Also, 1 could argue that registering and unregistering a {{Pool}} to be "managed" by a {{PoolManager}} is an independent concern of whether the {{Pool}} is in use or not. However, "closing" a {{Pool}} (especially, prematurely) is certainly a "management" concern for {{Pools}} that are registered (i.e should be "managed") with a manager (a.k.a. {{PoolManager}}). was (Author: jblum): This [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] (from this [commit|https://github.com/apache/geode/commit/51dd01cf309f1fccea9fe41e08fab42ed9759e52#diff-8d7c80296713855c7f737671ad251e9a]) would have been better designed and implemented with something like the following: 1) On the {{Pool}} interface, declare a {{isInUse():boolean}} method. {code:java} interface Pool { boolean isInUse(); ... } {code} 2) Then, in the {{PoolManagerImpl}} class, rather than exposing the internal implementation of {{PoolImpl}} for "how" a {{Pool's}} usage is tracked (i.e. with an {{attacheCount}} property and an {{AtomicInteger}}, as seen [here|https://github.com/apache/geode/blob/rel/v1.10.0/geode-core/src/main/java/org/apache/geode/cache/client/internal/PoolImpl.java#L1113-L1141] and [here|https://github.com/apache/geode/blob/rel/v1.10.0/geode-core/src/main/java/org/apache/geode/cache/client/internal/PoolImpl.java#L1102-L1108]; BTW, this *NOT* {{since}} *GemFire 5.7* and no references to GemFire should be in the Geode codebase in the first place!), the {{PoolManagerImpl}} can strictly adhere to the interface (this is what is referred to as "_programming to interfaces_", the "contract", and not the implementation). {code:java} if (pool.isInUse()) { throw new IllegalStateException("Pool could not be destroyed because it is still in use!"); } {code} Alternatively, and for convenience as well as "_encapsulation_", a method could be provided on the {{Pool}} interface to assert that a {{Pool}} is not in use. For instance: {code:java} interface Pool { boolean isInUse(); default void assertNotInUse() { throw new IllegalStateException("Pool is in use!"); } } {code} The {{assertNotInUse()}} method could be *overloaded* to take a String message as well. {code:java} interface Pool { boolean isInUse(); default void assertNotInUse() { assertNotInUse("Pool is in use!"); } default void assertNotInUse(String message) { throw new IllegalStateException(message); } } {code} The advantage of having this method on the {{Pool}} is that it allows the type of Exception thrown to be consistently applied everywhere in code that the usage of the Pool matters before acting. Additionally, it is assumed that a {{Pool}} could contain all the information necessary to craft informative and useful {{Exception}} message. I'd also argue that the original message stating "how many" Regions is useless information actually: {code:java} throw new IllegalStateException(String.format("Pool could not be destroyed because it is still in use by %s regions", attachCount)); {code} For debugging purposes, I'd rather know "which" Regions (by "name") were still use by the `Pool` in question. Additionally, the Exception message is actually misleading since in the {{PoolManagerImpl.unregister(:Pool)}} method, the {{Pool}} is not being "destroyed", it is simply being "unregistered" (i.e. "unmanaged"). Furthermore, it is NOT only Regions that use a {{Pool}}. It is also CQs and Functions, etc. Also, 1 could argue that registering and unregistering a {{Pool}} to be "managed" by a {{PoolManager}} is an independent concern of whether the {{Pool}} is in use or not. However, "closing" a {{Pool}} (especially, prematurely) is certainly a "management" concern for {{Pools}} that are registered (i.e should be "managed") with a manager (a.k.a. {{PoolManager}}). > 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)