-
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Celery doesn't re-use DB connections with Django (fixes #4116) #4292
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
Conversation
georgepsarakis
left a comment
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 would like to add that we use this fix successfully in production and hand no issues so far. |
|
Are there any plans for a patch fix (4.1.x) that will include this? |
|
@thedrow Is a 4.1.1 coming soon? Are there any PRs blocking it? |
|
4.2 is coming one blocker remain. check the milestone |
* setting is not respected by celery < v4.2 * celery/celery#4292 * not compatible with 'gevent' gunicorn worker class * benoitc/gunicorn#996 (comment)
|
this change is creating regression as per reports, should we revert this? |
|
@thedrow I read the issues linking here, I think that they mostly concern finding a correct value for |
|
This PR actually broke celery's DB handling (see #5483). I would suggest a revert. |
|
celery 4.x doesn't support Django-celery package |
|
Please review this patch witch django-celery-beats and results |
Fixes #4116
Celery doesn't re-use DB connections when the setting
CONN_MAX_AGEis set in Django. Instead connections are closed immediately. This leads to a high load on the database when there are a lot of Celery workers. It also affects query performance negatively.This PR uses the Django
close_if_unusable_or_obsolete()method to close a DB connection instead ofclose().close_if_unusable_or_obsolete()is available at least since Django 1.8 (see django/django@2830807) which is the minimum supported version for Celery 4.0.Let me know if you need anything else to merge this.