While changing a void method to have a return type does not break source compatibility, it appears likely to break binary compatibility[1].
So, if you are compiling your client from source, it will compile successfully against either Geode 1.12 or Geode 1.13. But if your client was already compiled [against Geode 1.12] and then you upgrade to Geode 1.13, without recompiling your client, I your client will throw MethodNotFoundException[2]. [1] https://wiki.eclipse.org/Evolving_Java-based_APIs_2#Evolving_API_interfaces_-_API_methods [2] https://stackoverflow.com/questions/47476509/can-i-change-a-return-type-to-be-a-strict-subtype-and-retain-binary-compatibilit > On Feb 27, 2020, at 5:09 PM, Kirk Lund <kl...@apache.org> wrote: > > Remember, if there are any concerns about recent backwards-compatible > changes to Geode user APIs, they should be brought on the dev list. > > Also, backward-compatible changes are by definition safe and ok for a user > API because it won't break the user's code. > > Here's an example of a user API that I recently fixed... > > The ClientRegionFactory is a builder with methods that are supposed to > follow fluent-style API (ie, return the ClientRegionFactory instance so you > can chain the calls). > > Whoever added setConcurrencyChecksEnabled goofed up and made the return > type void. Changing void to ClientRegionFactory is a fully backwards > compatible change which corrects the API. Since this fixes the API and > doesn't actually change the user API, this should be very safe and improves > Geode by correcting a broken API: > > *diff --git > a/geode-core/src/main/java/org/apache/geode/cache/client/ClientRegionFactory.java > b/geode-core/src/main/java/org/apache/geode/cache/client/ClientRegionFactory.java* > > *index add35f01a2..2a08307adc 100644* > > *--- > a/geode-core/src/main/java/org/apache/geode/cache/client/ClientRegionFactory.java* > > *+++ > b/geode-core/src/main/java/org/apache/geode/cache/client/ClientRegionFactory.java* > > @@ -216,7 +216,7 @@ public interface ClientRegionFactory<K, V> { > > * @since GemFire 7.0 > > * @param concurrencyChecksEnabled whether to perform concurrency checks > on operations > > */ > > - void setConcurrencyChecksEnabled(boolean concurrencyChecksEnabled); > > + ClientRegionFactory<K, V> setConcurrencyChecksEnabled(boolean > concurrencyChecksEnabled); > > > > /** > > * Sets the DiskStore name attribute. This causes the region to belong > to the DiskStore. > > *diff --git > a/geode-core/src/main/java/org/apache/geode/cache/client/internal/ClientRegionFactoryImpl.java > b/geode-core/src/main/java/org/apache/geode/cache/client/internal/ClientRegionFactoryImpl.java* > > *index 64256e8f8e..920deba055 100644* > > *--- > a/geode-core/src/main/java/org/apache/geode/cache/client/internal/ClientRegionFactoryImpl.java* > > *+++ > b/geode-core/src/main/java/org/apache/geode/cache/client/internal/ClientRegionFactoryImpl.java* > > @@ -186,8 +186,9 @@ public class ClientRegionFactoryImpl<K, V> implements > ClientRegionFactory<K, V> > > } > > > > @Override > > - public void setConcurrencyChecksEnabled(boolean > concurrencyChecksEnabled) { > > + public ClientRegionFactory<K, V> setConcurrencyChecksEnabled(boolean > concurrencyChecksEnabled) { > > > this.attrsFactory.setConcurrencyChecksEnabled(concurrencyChecksEnabled); > > + return this; > > } > > > > @Override > > Does anyone have concerns over fixing "void setConcurrencyChecksEnabled" > by changing it to "ClientRegionFactory setConcurrencyChecksEnabled"? If > there is a concern, is the concern about the change itself or because this > was fixed without following a more heavy-weight process? > > Thanks, > Kirk
