Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remedy breaking change with 2.x: Use CallbackAPIVersion.VERSION1 as a default #831

Merged
merged 1 commit into from
Apr 12, 2024

Conversation

amotl
Copy link
Contributor

@amotl amotl commented Mar 29, 2024

Dear @PierreF and @ralight,

first things first: Thanks a stack for conceiving and maintaining paho-mqtt.

We have been tripped by paho-mqtt 2.0.0 causing downstream havocs, because of incompatibilites. On pytest-mqtt, a corresponding minimal adjustment to make it compatible with both versions of paho-mqtt 1.x and 2.x looks like this:

Thoughts

As opposed to the rationale provided at #814 (comment), we think not introducing a breaking change on the most prominent spot will improve adoption rates, because users can run their code with both versions of paho-mqtt 1.x and 2.x, so it will be much easier to switch forth and back for conducting orientation flights on various levels of bringing code from development into production stages.

We don't think it will cause any harm, and other confusions. If the new API is documented properly, the word about it will progressively spread, and everyone will be happy?

We are probably wrong on many levels, not taking into account any discussions you certainly had about this topic. In this spirit, feel free to close this PR without further ado or notice.

Keep up the good work!

With kind regards,
Andreas.

@PierreF
Copy link
Contributor

PierreF commented Apr 7, 2024

Hi,

(reminder of ECA signature below)

thank for your feedback. I'm mitigated about the result. I indeed didn't think that much about code that NEED to handle both version of paho-mqtt (e.g. not end user of the library, but author of wrapper library / testing library).
But does you change will really helps handling the breaking changes ? It will only help in the most simple usage (and most simple tutorials, which I agree is a good point to don't break). Any code that use other arguments of Client() (e.g. client_id in mqttwarn case) will still need an update.

I'm not against this change, but I try to evaluate it's gain versus drawback. I currently only see:

  • gain: simple tutorials will continue to work
  • unchanged: any slightly more complex tutorials or code won't works either (e.g. client_id is used)
  • drawback: user that didn't upgraded yet will continue with CallbackAPI version 1 forever.

If the new API is documented properly, the word about it will progressively spread

I no longer believe in this. With or without your change, regardless of the documentation, some users will never upgrade to CallbackAPI version2. But this point make the above drawback moot. I hopped to don't have to support CallbackAPI version 1 forever, but this was a dream.

So to summarize, I would like to better understand how this will helps overcome the breaking change in more complex cases to see whether it's enough or if more should be made. But I guess this PR don't have actual drawback (maybe fewer users will upgrade to CallbackAPI version 2 ?) and make quickstart tutorials works which is nice.

reminder of ECA signature: btw, for your contribution to be accepted, could you sign the ECA please: https://accounts.eclipse.org/user/eca

@PierreF PierreF added this to the 2.1.0 milestone Apr 7, 2024
@amotl
Copy link
Contributor Author

amotl commented Apr 7, 2024

Dear Pierre,

thank you very much for your swift reply to our humble patch.

Does your change really help handling the breaking changes? It will only help in the most simple usage and most simple tutorials, which I agree is a good point to don't break.

Yeah, it will probably only help in those cases, and I admit I don't have deeper insights into more complex use cases, you are certainly the expert here.

I would like to better understand how this will helps overcome the breaking change in more complex cases to see whether it's enough or if more should be made.

I think it will be good enough, otherwise, things will probably become too complicated and/or confusing.

I guess this PR don't have actual drawback. Maybe fewer users will upgrade to CallbackAPI version 2? Making the quickstart tutorials work again is certainly nice.

Yes, our evaluation would be the same, but we don't think it will hold back users adopting CallbackAPI version 2. Serious users will want to use the most recent versions, but usually don't want to be forced into. Our recommendation would be to place prominent Python user warnings into the code paths you would like to phase out. This is a fair and convenient way to inform users about upcoming breaking changes, without breaking things immediately.

We believe it will do no harm to have those in v2, and then, if you want to get rid of backward compatibilities completely, you would follow up with a corresponding v3 release as soon as this fits your bill, it does not need to be years away, and could only be weeks if you like that idea.

With kind regards,
Andreas.

@PierreF
Copy link
Contributor

PierreF commented Apr 7, 2024

Did you see my note about signing the Eclipse contributor agreement ? This is something required by Eclipse to merge your contribution ?

@amotl
Copy link
Contributor Author

amotl commented Apr 7, 2024

I just signed the ECA, apparently successfully, thanks.

@amotl amotl force-pushed the callbackapi-default branch from 1a05ea7 to 2057c8b Compare April 7, 2024 18:43
@amotl
Copy link
Contributor Author

amotl commented Apr 7, 2024

Maybe I used a different email address, so it does not work. I am currently trying to change it, but I keep getting this message at https://accounts.eclipse.org/users/amotl/edit:

If you wish to use a different email address you must first change the primary email address associated with your account.

@amotl amotl force-pushed the callbackapi-default branch from 2057c8b to 38953bc Compare April 7, 2024 18:52
Use `callback_api_version = CallbackAPIVersion.VERSION1` as a default,
to not unnecessarily cause downstream havocs.

In this way, users can run their code with both versions of paho-mqtt
1.x and 2.x, which will significantly improve adoption rates.
@amotl amotl force-pushed the callbackapi-default branch from 38953bc to 5c77540 Compare April 7, 2024 18:54
@amotl
Copy link
Contributor Author

amotl commented Apr 7, 2024

I've managed to revoke the ECA, change the email address, and sign a new one. Apparently, it worked now.

@PierreF PierreF merged commit 6683aa6 into eclipse-paho:master Apr 12, 2024
10 checks passed
@amotl
Copy link
Contributor Author

amotl commented Apr 14, 2024

Dear Pierre,

thanks a stack for merging that patch. Now, paho-mqtt would need a quick bugfix release 2.0.1, so that people who are looking at, or are being pushed into upgrading, will be able to leverage that compatibility patch, i.e. not noticing anything when only using basic features, which was the cohort of people we have been targeting with this improvement, right?

With kind regards,
Andreas.

@PierreF
Copy link
Contributor

PierreF commented Apr 14, 2024

Absolutely, I've scheduled a 2.1.0 that I intend to release the 28 April: https://github.com/eclipse/paho.mqtt.python/milestone/6

The work currently left for that milestone is mostly fixing the flaky tests which I expect to do next week-end.

@amotl
Copy link
Contributor Author

amotl commented Apr 14, 2024

Dear Pierre,

may we ask why you decided to let users run into this issue for two more weeks? We would have expected this patch to make it into a bugfix release 2.0.1, and not into a point release.

With kind regards,
Andreas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants