If you must know, there are important test cases in both SBDG and SSDG to be able to register (and subsequently unregister) the "mock" Pool with the PoolManager, which unfortunately is a consequence of the SDG PoolFactoryBean's design being reliant on the PoolManager (to resolve the Pool), and to some extent, the PoolFactory API (to create the Pool) in the first place. Of course, there is no other way to "resolve" a reference to a Pool from Geode (other than ClientCache.geDefaultPool()), AFAIK.
Unfortunately, STDG (or SDG) would not need to import "internal" APIs if Apache Geode's public API was properly maintained and evolved to address a new use cases. For example (and again), see GEODE-7532 <https://issues.apache.org/jira/browse/GEODE-7532> [1]. It turns out, there are many, many cases like GEODE-7532 <https://issues.apache.org/jira/browse/GEODE-7532> [1], with classes and methods containing useful functions buried deep down inside Geode "internal" packages. The o.a.g.distributed.internal.MemberListener interface comes to mind. It is not difficult to see how his interface would be useful in both frameworks & tooling (e.g. for management & monitoring). Or, how about the poor resource management/cleanup Apache Geode fails to do properly w.r.t. SSL Sockets, that STDG now needs to account for <https://github.com/spring-projects/spring-test-data-geode/blob/master/spring-data-geode-test/src/main/java/org/springframework/data/gemfire/tests/integration/IntegrationTestsSupport.java#L119-L139> [2] when running automated integration tests. I think there was another example <https://markmail.org/message/5juxxnkttzjndidg> [3] recently, which STDG must account for <https://markmail.org/message/5juxxnkttzjndidg> [4] as well. Serializer registration/unregistration is yet another area. The laundry list is quite long! The whole notion of Apache Geode's "internal" API is horribly broken, especially when the public API often comes up short. What troubles me is that we see the recommended changes in GEODE-7532 as a "bandaid". Really? It does not matter whether these classes are "internal" or not, nor whether they are used (externally) or not, so long as they are used properly. If they are "public" there is simply no guarantee they won't be used, and you cannot expect "internal" problems to not have external consequences. SIDE NOTE: Of course, this is further predicated on the fact those "internal" classes were designed/implemented correctly in the first place, which they also, were not! I down voted for several reasons: 1. First, YES, because this blocks SBDG from moving to Apache Geode 1.11, and perhaps, more importantly... 2. Because the Pool management in PoolManagerImpl A) refers to the Pool as PoolImpl despite taking a reference to Pool, and B) exposes the implementation of the usage tracking logic (which is a clear violation of "encapsulation" and a series code smell) 3. And because the IllegalStateException message is not accurate nor was it very informative (e.g. "what Regions"). 4. Or because 2A by (along with GEODE-7159 <https://issues.apache.org/jira/browse/GEODE-7159> [5]) will most definitely cause problems for us later, especially since these problems will be time delayed (invoked during resource cleanup) making them hard to pinpoint and very confusing to users. Also, imagine a scenario where Geode offers different Pool implementations, beyond PoolImpl. Why else would Geode provide a PoolFactory interface if not to encapsulate creation, configuration and initialization of different types of Pools.... part of that *Open/Closed* principle. There is no point to have a PoolFactory otherwise since the PoolManager could have simply created Pools. Ironically, the PoolFactory is not something that is used in this way and therefore became an unnecessary layer of indirection. #sigh In addition, the recommended changes in GEODE-7531 are not invasive what-so-ever, and will not adversely affect Geode 1 way or another. In closing, anytime the overall code quality of Geode comes into question (and there is a lot to question with this codebase), which can, and most likely will affect our end-users experience, and it HAS and DOES, you better believe and I am going to call this out. -j [1] https://issues.apache.org/jira/browse/GEODE-7532 [2] https://github.com/spring-projects/spring-test-data-geode/blob/master/spring-data-geode-test/src/main/java/org/springframework/data/gemfire/tests/integration/IntegrationTestsSupport.java#L119-L139 [3] https://markmail.org/message/5juxxnkttzjndidg [4] https://markmail.org/message/5juxxnkttzjndidg [5] https://issues.apache.org/jira/browse/GEODE-7159 On Wed, Dec 4, 2019 at 6:12 PM Robert Houghton <rhough...@pivotal.io> wrote: > @udo, if one needs to use a strong word like 'offender', then the offender > is the one using an internal API. Geode is under no obligation to maintain > or "fix" these for any project. > > Is there a Jira, github issue, or pull-request to promote the internal > class to the public space? Is there a feature request to uncouple this > class you want to use in Spring? > > Is Spring the tail wagging the dog for Geode releases? > > > On Wed, Dec 4, 2019, 17:59 Udo Kohlmeyer <u...@apache.com> wrote: > > > @Dan, > > > > I will add my -1 to this. > > > > I understand your argument of "let's solve the problem by removing the > > offender". > > > > But in reality who is the offender? Is it the one class that is using an > > "internal" api OR is it the implementation itself that is to tightly > > coupled that extending it is impossible? > > > > STDG right now is trying to create a test, to test functionality that > > requires it to inject/mock a Pool. The smallest piece of the code, not > > the WHOLE PoolManager. But the system does not allow for the injection > > of a `Pool` (which btw all the methods on the `PoolManagerImpl` has as > > arguments). It casts every generic `Pool` to its implementation. This is > > a very limiting factor. > > > > I understand that delaying a release is not optimal, but how much effort > > to we believe it to be to clean up the code that so tightly couples > > implementations together. > > > > I think we also must not forget that John, like many of us, is a > > committer and PMC member on Geode. He also happens to be the main > > committer on SDG, SBDG, SSDG, STDG. But if he feels that a release > > should not go out without fixing a limiting feature to the Geode > > project, then he may do so. > > > > I also feel that this is a limiting factor. > > > > The only difference is that I am not the main committer on the Spring > > data projects for Geode. John is merely trying to get the best outcome > > for both projects. > > > > --Udo > > > > On 12/4/19 4:39 PM, Dan Smith wrote: > > >> Quite frankly the reasons STDG (or dependent projects downstream like > > SDG, > > >> SBDG, SSDG) are doing what it is (they are) doing is irrelevant to > point > > >> articulated in the description of GEODE-753. > > >> > > > What bothers me here is not your suggestions in GEODE-1753, but the > fact > > > that you are vetoing a Geode release because STDG is using an internal > > > Geode method and had a problem. > > > > > > How hard is it to remove the use of PoolManagerImpl from STDG? If we > can > > > fix the issue there, that seems better than putting bandaids into the > > > release branch so STDG can continue to use internal APIs. > > > > > > -Dan > > > > > > -- -John Spring Data Team
