-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
upgrade celery 5.2.3 #19703
upgrade celery 5.2.3 #19703
Conversation
let me know what else needs to be updated |
That should be enough @auvipy ! Looks good all tests passed. Anyone knows any potential problem involved with it ? |
actually it contains several memory leaks/usage issues fixes in amqp & redis & 1 moderate but very old security issue. |
setup.py
Outdated
'celery~=5.1,>=5.1.2', | ||
'celery~=5.2,>=5.2.3', |
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.
Can this not continue to allow Celery 5.1.x by declaring celery>=5.1
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.
we usually only support the latest stable version in celery & https://github.com/celery/celery/releases/tag/v5.2.2 is not back ported.
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.
IF this one contains some security issue and memory leaks, then I am ok with >=5.2.3.
But I thought a bit and we should really remove the upper limit - this is our default behaviour and our constraint mechanism will keep it in check but we will avoid any need for future changes like that for 5.3 and above.
setup.py
Outdated
@@ -227,7 +227,7 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version | |||
'cassandra-driver>=3.13.0,<4', | |||
] | |||
celery = [ | |||
'celery~=5.1,>=5.1.2', | |||
'celery~=5.2,>=5.2.3', |
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.
'celery~=5.2,>=5.2.3', | |
'celery>=5.2.3', |
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.
done
The error is something I will have to fix - nothing to worry about (aftermath of #20624 - which I was not able to quickly test). |
I will upgrade constratints and merge this one. |
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. |
Actually we were already at 5.2.3 so it was not needed to upgrade constraints before merge |
thanks. btw I have triage access to github, can I use https://issues.apache.org/jira/projects/AIRFLOW/issues/AIRFLOW-4470?filter=allopenissues as well? any instructions? I wish to contribute to good first issues |
The Issues is JIRA are not used any more - look at the https://github.com/apache/airflow/contribute instead. The instrructions for contribution are thre too (top right CONTRIBUTING.rst) link :). Also you might take a look at a contribution workshop we held last year; https://youtu.be/kvccZizzfTk |
ok though signed up! will check the video |
You can find good first issues with this link |
FYI this version increase includes some much needed fixes regarding memory leaks in Celery w/ Redis brokers. We were experiencing our airflow workers failing from memory leaks due to a problem with health checks - after some further investigation it seems due to celery/celery#4843. Pretty good description of the issue in celery/celery#4843 (comment) For our side, a fix was made in Kombo 5.2.3 and included in Celery 5.2.3 which resolve the memory leak issue: A change was also included to shrink the overall memory usage of For an overall review of what was changed to resolve the memory leaks, please see celery/celery#4843 (comment) I'm not sure regarding your release cadence, but this includes some pretty big fixes. |
Consider it done! As mentioned before - our automated uggrade process already picked it up and I am also just about to cherry-pick and refresh constraints and docker images for 2.2 branch for the upcoming 2.2.4 release (weeks rather than months). |
really happy to know this! |
can you please try celery/py-amqp#368 and linked PR? I will be very grateful |
(cherry picked from commit e1fbfc6)
(cherry picked from commit e1fbfc6)
this update is very much needed for several reason
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.