-
Notifications
You must be signed in to change notification settings - Fork 14.8k
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
Remove Provider Deprecations in Common SQL #44645
Remove Provider Deprecations in Common SQL #44645
Conversation
Co-authored-by: Jens Scheffler <95105677+jscheffl@users.noreply.github.com>
2ee6205
to
107caf7
Compare
* Remove Provider Deprecations in Common SQL * Fix Changelog header Co-authored-by: Jens Scheffler <95105677+jscheffl@users.noreply.github.com> --------- Co-authored-by: Jens Scheffler <95105677+jscheffl@users.noreply.github.com>
* Hooks | ||
* Remove ``_make_serializable`` method from ``DbApiHook``. Use ``_make_common_data_structure`` instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need more context here?
To a user it might seem like we remove private internal function.
I don't know what it's used for but either it's not part of the public API and we shouldn't have deprecate it to begin with or it has user impact and we need to better explain what it is?
(To clarify since we already deprecated we will have to cut breaking release with remove not but the change log should better indicate what is the change)
similar to what I commented on #44567 (comment)
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 no conclusiosns (except renaming of _make_serializable
) discussion 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also decide to exclude common.sql and do the removal at later phase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to agree that we should just tweak the message on the change log
* Remove Provider Deprecations in Common SQL * Fix Changelog header Co-authored-by: Jens Scheffler <95105677+jscheffl@users.noreply.github.com> --------- Co-authored-by: Jens Scheffler <95105677+jscheffl@users.noreply.github.com>
* Remove Provider Deprecations in Common SQL * Fix Changelog header Co-authored-by: Jens Scheffler <95105677+jscheffl@users.noreply.github.com> --------- Co-authored-by: Jens Scheffler <95105677+jscheffl@users.noreply.github.com>
related: #44559
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rst
or{issue_number}.significant.rst
, in newsfragments.