potiuk commented on code in PR #44645: URL: https://github.com/apache/airflow/pull/44645#discussion_r1878392885
########## providers/src/airflow/providers/common/sql/CHANGELOG.rst: ########## @@ -25,6 +25,16 @@ Changelog --------- +main +..... + +.. warning:: + All deprecated classes, parameters and features have been removed from the Common SQL provider package. + The following breaking changes were introduced: + + * Hooks + * Remove ``_make_serializable`` method from ``DbApiHook``. Use ``_make_common_data_structure`` instead. Review Comment: OK. So the thing is - we should basically never (unless we have a very good reason) make a breaking release of a common provider. If we do - this breaks a lot of things when it comes to provider up/downgrability. This means that if ONE provider will start using features from such "breaking change" release. other providers will have to use it as well. So if the other providers depend on the "broken" feature, they will be broken once you upgrade common.sql to the breaking version. That's one of the difficulties with "common" - because in Python (unlike in npm or system libraries) we cannot have two versions of the same library used at the same time. I'd say this change is "lightly breaking" - it's likely breaking single (or very few) versions of past released providers. It does not break user code, it breaks some old version of providers (and we could consider that as a bugifix. We could likely try to determine which versions of which providers were affected by this change. There were two heated but otherwise resulting in nothing (except renaming of `_make_serializable`) conclusions a year ago in December: * https://lists.apache.org/thread/28r9x52cc51tcfxc5c6n3vh9frgp287x * https://lists.apache.org/thread/z4sh6v9bnxhpgggxos8d0rc705tpz1k5 Having common.sql incompatibility and potential impact on already released providers was one of the issues I raised then as a reason why we should not rename it. We added deprecation instead. IMHO year is long enough to consider those "old providers" are "out of fashion" and assessing that it's highly unlikely somoene will have new `common.sql` and very old one of the providers that depend o `common.sql` - with this specific `_make_serializable` used. It's also very easy to fix such issue (by upgrading the affected provider). So for me I'd not classify it as a breaking change but "misc" and accept the fact that some past providers might be broken. Alternative is to add `2.` version of the `common.sql` (and it would likely have no big impact because the same case - probability of breaking change is small) but I'd say the reason for bumping to 2.* is not too strong. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
