If I run the japi-compliance-checker [1] against the 1.12 release branch and develop this change pops up as a binary incompatibility. As Owen noted, this would require recompilation to avoid NoSuchMethod errors.
One effect this could have is that a library built on top of Geode (e.g. Spring) would not be compatible with both 1.12 and 1.13. Anthony [1] https://lvc.github.io/japi-compliance-checker/ <https://lvc.github.io/japi-compliance-checker/> > On Feb 27, 2020, at 5:19 PM, Owen Nichols <onich...@pivotal.io> wrote: > > 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 >
