Ok, thanks! I've submitted https://github.com/apache/geode/pull/4748 to revert the change. I'll be offline for a while so feel free to merge it if it passes.
Thanks, Kirk On Thu, 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 > >
