+1 for a short-term solution in 1.11 while we discuss a more complete proposal for 1.12
On Wed, Dec 4, 2019 at 10:09 PM Jacob Barrett <jbarr...@pivotal.io> wrote: > 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 > >
