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

Remove deprecations in airflow.models.taskreschedule #41808

Conversation

jscheffl
Copy link
Contributor

Code cleanup and removal of deprecations in airflow.models.taskreschedule.

@jscheffl jscheffl force-pushed the feature/remove-deprecations-in-airflow-models-taskreschedule branch from 59c654c to cf8d291 Compare August 28, 2024 21:10
@jscheffl jscheffl requested a review from vincbeck August 28, 2024 21:11
@jscheffl jscheffl merged commit 6c99f7c into apache:main Aug 28, 2024
50 checks passed
@vikramkoka
Copy link
Contributor

@jscheffl

It's not that I disagree with the PR, but I am surprised by the comment in the newsfragment, regarding the use of sqlalchemy. Since we are not disallowing direct access to the DB, wouldn't that be problematic?

@jscheffl
Copy link
Contributor Author

@jscheffl

It's not that I disagree with the PR, but I am surprised by the comment in the newsfragment, regarding the use of sqlalchemy. Since we are not disallowing direct access to the DB, wouldn't that be problematic?

@vikramkoka The cleanup of the deprecation was not driven by a function or demand to urgently remove it. It was in a batch of "make the house clean" rounds in search & remove all deprecations. The both mentioned functions were not used in any area in the code anyway, they just have kept for a long time as legacy interface - with the reason of semantic release and non-breaking.

I did not check the reason behind it and if there is any benefit in keeping them. Somebody in the past long time ago marked them deprecated and this was just a cleaning exercise. And yes, they were not used internally a long time ago and therefore there was no replacements. Did not make a forensic analysis and I assume everybody using that from the past might just copy the traces and re-implement rather having a public API on these helpers. Also as no forensic... I don't see a problem - but this is not a rationale to keep deprecations. I assume they are a leftover trace of a re-factoring which still harm future development.

@potiuk
Copy link
Member

potiuk commented Nov 21, 2024

Yeah. I think @vikramkoka is right - newsfragment in this one is a bit misleading. Since news fragments are intended for users - I think we should clean up the description a bit and say 'direct access to DB is discouraged - using REST API is the only way to interact with Airflow'.

This is our statement for a long time and we captured it in the 'Public Interface'.

@jscheffl jscheffl deleted the feature/remove-deprecations-in-airflow-models-taskreschedule branch November 21, 2024 20:44
@jscheffl
Copy link
Contributor Author

Yeah. I think @vikramkoka is right - newsfragment in this one is a bit misleading. Since news fragments are intended for users - I think we should clean up the description a bit and say 'direct access to DB is discouraged - using REST API is the only way to interact with Airflow'.

This is our statement for a long time and we captured it in the 'Public Interface'.

Sorry took a moment reading it a second time. I interpreted the first remark as a question "why" we remove the deprecation and now realized that the point to sqlalchemy was the problem you wanted to highlight :-D

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.

4 participants