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

perf: respect django database routing #451

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

systemime
Copy link

@systemime systemime commented Nov 2, 2021

Summary:

  • Celery Version: 5.1.2
  • Celery-Beat Version: 2.2.1

The problem you want to solve

If the django project is configured with multiple databases, the default behavior in the django-celery-beat will cause django to get the default database connection, but at this time it is an empty dictionary, and then a database configuration exception will be thrown. This pr will check the DATABASE_ROUTERS to determine whether django db router is needed.

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you also add relevant test ?

@auvipy
Copy link
Member

auvipy commented Dec 15, 2021

@D3X @lvelvee can you try this pr to verify it won't introduce any regression?

@auvipy auvipy added this to the v2.3.0 milestone Jan 3, 2022
@systemime systemime force-pushed the systemime/perf_respect_django_routing branch from ab8f2d9 to e718a8b Compare January 20, 2022 04:50
@systemime systemime closed this Jan 20, 2022
@systemime systemime reopened this Jan 20, 2022
@systemime
Copy link
Author

Sorry, I'm trying to add unit tests for here but don't have any good ideas. I referenced Django's unit tests and used the override_settings decorator to simulate the multidb case. By default, the Router's db_for_write method returning None will cause Django to use the default database configuration, regardless of whether the user is using multidb or not.

@auvipy

@auvipy
Copy link
Member

auvipy commented Jan 20, 2022

using django unit test is a good start, I will take some time to properly review & test it before merge

@auvipy auvipy requested review from CameronSima and a team and removed request for CameronSima January 20, 2022 05:14
@auvipy auvipy closed this Jun 4, 2022
@auvipy auvipy reopened this Jun 4, 2022
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests are failing. also please rebase

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.

None yet

2 participants