+1 to what Jake said. -Dan
On Wed, Dec 4, 2019 at 11:32 PM Owen Nichols <onich...@pivotal.io> wrote: > +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 > > > > >
