-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
Add cache_ok flag to sqlalchemy TypeDecorators. #24499
Conversation
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
|
Fixes new issue #24504 . |
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.
LGTM. But @ephraimbuddy @dstandish or @ashb co-review is needed
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
45d0b1b
to
91beef1
Compare
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 at least ExtendedJSON
should use cache_ok = False
since the value is not cachable? (Not sure about relativedelta
.)
The See this section of the SQLAlchemy docs on how cache keys are generated. Using a |
91beef1
to
9d8ed02
Compare
Makes sense . I rebased to see if it will be fine. also it might be necesary after #24819 is merged as we will be upgrading to sqlalchemy > 1.4.39 that will start emit this warning. |
Just in case - we should rebase this one after #24819 gets merged to test its behaviour anyway. |
Awesome work, congrats on your first merged pull request! |
(cherry picked from commit d969473)
(cherry picked from commit d969473)
This fixes #22647, which was marked closed due to lack of activity / feedback.
SQLAlchemy added the
cache_ok
flag in 1.4.14. If this is unset in aTypeDecorator
subclass, it emits a log warning when the class is used. The fix is to set the value toTrue
orFalse
to suppress the warning.I've set the value to
True
for the threeTypeDecorator
classes in theairflow.utils.sqlalchemy
module, as these classes have no state and are safe to cache. Setting it toFalse
would also suppress the warning, and leave behavior unchanged.