Thanks Kirk and team! +1 for bubbling up some of the internal APIs and having consistency and/or functional parity.
One suggestion is, perhaps a quick note could be sent out to the Geode devs with a title/subject of "*NOTICE: Frameworks/Tooling*" with a body detailing the API (public/internal) changes that would affect 3rd party projects (e.g. access modifier changes, removing methods, changing signatures, introducing new types/properties, etc) Another suggestion... it might be nice to list the more formal/official projects, like SDG, that the community is working on for the Apache Geode ecosystem. I literally have no idea what additional projects exist that might benefit the community. *Pivotal GemFire* could be even counted among these ecosystem projects/products.... perhaps another section similar to " *Deployments*" on the Community page <http://geode.apache.org/community/> [1]. Food for thought. Thanks, John [1] http://geode.apache.org/community/ On Thu, May 18, 2017 at 10:31 AM, Kirk Lund <kl...@apache.org> wrote: > I think we should definitely try to refactor any Geode internals that SDG > is using to be public API or SPI. We'll need to take a closer look at how > SDG is using those classes to plan what to do. > > If I change or any classes on your list (or public APIs), I'll give you > heads up next time. > > On Thu, May 18, 2017 at 10:21 AM, John Blum <jb...@pivotal.io> wrote: > > > Hi Kirk (also responding to Bruce as I was writing this before he sent)- > > > > Thank you for following up. > > > > Regarding the addition of the getOnlineLocators()... > > > > SDG does implement > > <http://docs.spring.io/spring-data-gemfire/docs/current/api/ > > org/springframework/data/gemfire/client/support/package-summary.html> > > [1] > > a few Geode interfaces, such as *org.apache.geode.cache.client.Pool*. > > This > > is due in part since not all the state for certain Geode objects (e.g. > > Pool) is known in advance as the actual object may not have been created > by > > the *Spring* container yet. However, that state is captured in *Spring* > > FactoryBeans, which will be used to construct the actual object at the > > appropriate time. > > > > Anyway, like the "internal" API, I have tried to keep such > implementations > > of Geode interfaces/classes to a minimum. Still, it is a good indicator > of > > when something has changed since I will most certainly get a compilation > > failure. This leads me to, I do need to know when APIs change to provide > > appropriate support in SDG, possibly in configuration, whether XML of via > > Java/annotations with the FactoryBeans, e.g. PoolFactoryBean > > <http://docs.spring.io/spring-data-gemfire/docs/current/api/ > > org/springframework/data/gemfire/client/PoolFactoryBean.html> > > [2], > > especially if it is a configuration property/attribute. Other cases my > be > > important from an accessibility or information standpoint (probably more > so > > for tooling). > > > > As for the InternalDataSerializer... > > > > No worries. I found an acceptable and actually better workaround. > Again, > > I am trying to reduce or completely eliminate the use of "internal" > classes > > in SDG. Unfortunately, a quick search (using regex) in SDG for "import > > org\.apache\.geode.*internal" reveals 35 occurrences, the most pertinent > > ones being... > > > > org.apache.geode.internal.GemFireVersion (used in debugging) > > org.apache.geode.internal.DistributionLocator > > org.apache.geode.internal.InternalDataSerializer > > org.apache.geode.internal.cache.GemFireCacheImpl > > org.apache.geode.internal.cache.LocalRegion (used to inspect whether a > > server proxy is present to acquire the appropriate QueryService) > > org.apache.geode.internal.cache.PartitionedRegion > > org.apache.geode.internal.cache.UserSpecifiedRegionAttributes > > org.apache.geode.internal.cache.lru.LRUCapacityController > > org.apache.geode.internal.cache.lru.MemLRUCapacityController > > org.apache.geode.internal.cache.query.internal.ResultsBag > > org.apache.geode.internal.distributed.internal.DistributionConfig > > org.apache.geode.internal.distributed.internal.InternalDistributedSystem > > org.apache.geode.internal.distributed.internal.InternalLocator > > org.apache.geode.internal.datasource.ConfigProperty (JNDI support) > > org.apache.geode.internal.jndi.JNDIInvoker (JNDI support) > > org.apache.geode.internal.security.SecurityService (my doing) > > > > Most of this was added before my time. I just need time to review these > > more carefully, and how each is used. I know many of these are used for > > good reason. > > > > Thanks, > > John > > > > > > [1] > > http://docs.spring.io/spring-data-gemfire/docs/current/api/ > > org/springframework/data/gemfire/client/support/package-summary.html > > [2] > > http://docs.spring.io/spring-data-gemfire/docs/current/api/ > > org/springframework/data/gemfire/client/PoolFactoryBean.html > > > > > > On Thu, May 18, 2017 at 10:18 AM, Bruce Schuchardt < > bschucha...@pivotal.io > > > > > wrote: > > > > > John, is it possible to list the internal APIs needed by SDG? I don't > > > think we can live with a requirement that all internal API changes need > > to > > > be communicated and blessed. We need to address SDG's needs in the > > public > > > APIs and get rid of this dependency. > > > > > > > > > Le 5/18/2017 à 9:36 AM, Kirk Lund a écrit : > > > > > >> Hi John! How did the addition of getOnlineLocators() cause SDG build > to > > >> fail? I checked [9] but I couldn't quite grok what happened. I > would've > > >> thought that adding would be ok but removing could potentially break > > SDG. > > >> > > >> The InternalDataSerializer change was probably mine. I reduced scope > of > > >> methods and variables where possible. Sorry about that. Do you need me > > to > > >> restore getSerializer to public? > > >> > > >> On Wed, May 17, 2017 at 4:42 PM, John Blum <jb...@pivotal.io> wrote: > > >> > > >> Geode devs- > > >>> > > >>> I am not sure how decisions are made when changing Geode's API, but I > > >>> would > > >>> caution that more care should be taken when doing so, regardless of > > >>> whether > > >>> the APIs are public or not, but especially when they are public. > > >>> > > >>> Unfortunately, in this particular case, the API in question is deemed > > >>> "internal", which I know, are subject to change. However, the > problem > > >>> with > > >>> this is, the public API is insufficient in some cases, particularly > for > > >>> "frameworks" (e.g. SDG) and "tooling" building on Geode and > attempting > > to > > >>> uphold Geode's functional/behavioral contract. > > >>> > > >>> By way of example, a recent *Spring Data Geode* build broke > > >>> <https://build.spring.io/browse/SGF-NAG-556/log> [1] because, the > > >>> org.apache.geode.internal.InternalDataSerializer class was recently > > >>> changed. The scope of getSerializer(:Class):DataSerializer was > changed > > >>> from *public > > >>> <https://github.com/apache/geode/blob/rel/v1.1.1/geode- > > >>> core/src/main/java/org/apache/geode/internal/InternalDataSer > > >>> ializer.java# > > >>> L1113>* > > >>> [2] > > >>> to *private > > >>> <https://github.com/apache/geode/blob/a44585316a8d7eed35917c1dfd8293 > > >>> 15b34ce316/geode-core/src/main/java/org/apache/geode/internal/ > > >>> InternalDataSerializer.java#L1090>* > > >>> [3] > > >>> in a seemingly unrelated commit. > > >>> > > >>> There is a class > > >>> <https://github.com/spring-projects/spring-data-geode/ > > >>> blob/master/src/main/java/org/springframework/data/gemfire/ > > >>> serialization/EnumSerializer.java#L39> > > >>> [4] > > >>> in SDG that extends DataSerializer > > >>> <http://gemfire-90-javadocs.docs.pivotal.io/org/apache/ > > >>> geode/DataSerializer.html> > > >>> [5] > > >>> (in Geode's public API) but must use the InternalDataSerializer > > (internal > > >>> Geode API) when changes (e.g. "supported classes") to the SDG > > serializer > > >>> need to be "distributed" across the cluster. SDG"s serializer use to > > >>> call > > >>> this getSerializer(:Class):DataSerializer method. However, in this > > >>> case, > > >>> I > > >>> fixed the problem by not calling this "overloaded" method as it was > not > > >>> strictly necessary. > > >>> > > >>> While I am trying to avoid the use of "internal" Geode classes and > APIs > > >>> in > > >>> SDG in general, as I mention above, this is not always possible. For > > >>> instance, there are a few API blunders such as the ability to > register > > >>> <http://gemfire-90-javadocs.docs.pivotal.io/org/apache/ > > >>> geode/DataSerializer.html#register-java.lang.Class-> > > >>> [6] > > >>> a DataSerializer class but not "unregister" it; that is in > > >>> <https://github.com/apache/geode/blob/rel/v1.1.1/geode- > > >>> core/src/main/java/org/apache/geode/internal/InternalDataSer > > >>> ializer.java# > > >>> L1077> > > >>> [7] > > >>> InternalDataSerializer. The other bad API practices on this class > > (i.e. > > >>> (Internal)DataSerializer) alone. > > >>> > > >>> Framework and tool developers must occasionally rely on "internal" > APIs > > >>> (especially when the public API is insufficient) to order to achieve > a > > >>> similar experience to Geode alone; something to keep in mind as > Geode's > > >>> ecosystem grows. > > >>> > > >>> Finally, I'll also add that, I do need to know anytime the public API > > >>> changes as well. Recently the getOnlineLocators() method > > >>> <https://github.com/apache/geode/blob/61f676b0187e8918ca62f4a2ec9b6b > > >>> d3baa942d3/geode-core/src/main/java/org/apache/geode/ > > >>> cache/client/Pool.java#L210> > > >>> [8] > > >>> was added to org.apache.geode.cache.client.Pool, which caused > another > > >>> SDG > > >>> build <https://build.spring.io/browse/SGF-NAG-552> [9] to fail. > > >>> > > >>> Special thank you to *Nabarun* and *Diane* for the recent heads about > > the > > >>> LuceneQueryFactory method name change... from setResultLimit(:int) to > > >>> setLimit(:int). > > >>> > > >>> Regards, > > >>> > > >>> -- > > >>> -John > > >>> > > >>> > > >>> [1] https://build.spring.io/browse/SGF-NAG-556/log > > >>> [2] > > >>> https://github.com/apache/geode/blob/rel/v1.1.1/geode- > > >>> core/src/main/java/org/apache/geode/internal/InternalDataSer > > >>> ializer.java# > > >>> L1113 > > >>> [3] > > >>> https://github.com/apache/geode/blob/a44585316a8d7eed35917c1dfd8293 > > >>> 15b34ce316/geode-core/src/main/java/org/apache/geode/internal/ > > >>> InternalDataSerializer.java#L1090 > > >>> [4] > > >>> https://github.com/spring-projects/spring-data-geode/ > > >>> blob/master/src/main/java/org/springframework/data/gemfire/ > > >>> serialization/EnumSerializer.java#L39 > > >>> [5] > > >>> http://gemfire-90-javadocs.docs.pivotal.io/org/apache/ > > >>> geode/DataSerializer.html > > >>> [6] > > >>> http://gemfire-90-javadocs.docs.pivotal.io/org/apache/ > > >>> geode/DataSerializer.html#register-java.lang.Class- > > >>> [7] > > >>> https://github.com/apache/geode/blob/rel/v1.1.1/geode- > > >>> core/src/main/java/org/apache/geode/internal/InternalDataSer > > >>> ializer.java# > > >>> L1077 > > >>> [8] > > >>> https://github.com/apache/geode/blob/61f676b0187e8918ca62f4a2ec9b6b > > >>> d3baa942d3/geode-core/src/main/java/org/apache/geode/ > > >>> cache/client/Pool.java#L210 > > >>> [9] https://build.spring.io/browse/SGF-NAG-552 > > >>> > > >>> > > > > > > > > > -- > > -John > > john.blum10101 (skype) > > > -- -John john.blum10101 (skype)
