I think we can tone down the inflammatory statements. It is well established that, like any legacy code base, Geode has issues. One of them is a less than ideal set of APIs for certain tasks. Whatever the issues were in the past with getting APIs adjusted to suit the SDG project should be left in the past and we can work forward on a new strategy.
The term public API is redundant, if its designated API then it is public. Let’s also be clear that "internal API” is not a thing, if it is internal it is not an API. The language semantics of public/private are not what defines and API. As such, relying on internal classes tor work around deficiencies in a API is dangerous and should only be done as a stopgap measure while addressing the API’s deficiency. Java, up until 9, lacked any clear distinction of internals vs API so, like many projects, Geode chose the “internal” package name to denote things that are not to be consume as API. As we go forward with Java 9 modules support these will be enforced by the runtime and compilers. A JIRA should be opened to address the API deficiency. When the API is enhanced then the internal class usage should be replaced immediately with the API. It is unreasonable to expect an upstream project to maintain internal classes as though they are API or expect them to address deficiencies in the API if they are not communicated. It is also unreasonable for the upstream project to ignore obvious deficiencies in its APIs and should use these issues raised by downstream projects to prioritize improvements. Work needs to be done on both these projects to make improvements. To resolve the issues raised below let’s get some JIRAs and/or RFCs opened to address the APIs the SDG needs to work. Let’s work to burn down this debt of internal class usage in SDG to avoid issues like this in the future. I am sure we can figure out a short term solution to address the immediate compile issues, whether that’s making a unit test into an integration test, refactoring the internals to not blindly cast, or whatever. We can then work out a strategy to address this the correct way in the develop branch for 1.12. -Jake > On Dec 4, 2019, at 6:20 PM, John Blum <jb...@pivotal.io> wrote: > > 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
