Another affect is code deployment onto/into the server, which
could/would reference a change (binary) API. Users generally don't
recompile the code they redeploy. The NoSuchMethod is now harder to
track down.
--Udo
On 2/28/20 8:59 AM, Anthony Baker wrote:
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
