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) >
