You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Kirk Lund <kl...@apache.org> on 2020/02/28 01:09:30 UTC
Discussions about concerns over User API changes
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
Re: Discussions about concerns over User API changes
Posted by Udo Kohlmeyer <ud...@apache.com>.
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 <on...@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
>
Re: Discussions about concerns over User API changes
Posted by Anthony Baker <ab...@pivotal.io>.
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 <on...@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
>
Re: Discussions about concerns over User API changes
Posted by Kirk Lund <kl...@apache.org>.
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 <on...@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
>
>
Re: Discussions about concerns over User API changes
Posted by Owen Nichols <on...@pivotal.io>.
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