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

Fix #123 -- init broker configuration before loading other app models #126

Merged
merged 3 commits into from
Nov 5, 2022
Merged

Fix #123 -- init broker configuration before loading other app models #126

merged 3 commits into from
Nov 5, 2022

Conversation

amureki
Copy link
Collaborator

@amureki amureki commented Oct 31, 2022

Bug from #123 was introduced in #103 where django_dramatiq started benefitting from AppConfig.ready() functionality.
There was a research done in #100 explaining the underlying issue.

My PR is using the idea from @dnmellen to call DjangoDramatiqConfig.ready() during its models import, so earlier than any other custom models code would be imported.
I was trying to come up with a different way, as this one feels tiny bit "hacky", however could not find anything better.

@amureki
Copy link
Collaborator Author

amureki commented Oct 31, 2022

@Bogdanp hey Bogdan! Would be happy to get your input here. Also, would be great to trigger CI jobs to check the PR.

Best,
Rust

@Bogdanp
Copy link
Owner

Bogdanp commented Nov 1, 2022

Thanks! This looks reasonable. I’ll take a deeper look this Sunday. I don’t see an option to trigger CI right now. Maybe the workflow isn’t set up to run on PRs (if not, that would also be a good change to make).

@amureki
Copy link
Collaborator Author

amureki commented Nov 2, 2022

Something is odd with locks in test database. Seems flaky to me, but I am not yet familiar enough with the codebase to quickly find out the reason.

django.db.utils.OperationalError: database table is locked: django_dramatiq_task

Copy link
Owner

@Bogdanp Bogdanp left a comment

Choose a reason for hiding this comment

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

Looks good, I just had a question about ready.

def ready(self):
if getattr(self, "_is_ready", False):
super().ready()
Copy link
Owner

Choose a reason for hiding this comment

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

Can you explain this a little bit? Is this needed because now that we're calling ready from import_models, it's also likely to be called during the regular django app lifecycle? If so, wouldn't it be better to avoid running the code at all the second time it's called? I.e.:

def ready(self):
  if getattr(self, "_is_ready", False):
    return
  super().ready()
  ...
  self._is_ready = True

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, the idea is to initialize this code during import_models, and then not to rerun it again. Your note is correct, in this case we don't want anything to be called second time, including super().ready(). I missed it as AppConfig.ready is not doing anything, but good point!

tests/testapp1/apps.py Outdated Show resolved Hide resolved
@Bogdanp
Copy link
Owner

Bogdanp commented Nov 5, 2022

Something is odd with locks in test database. Seems flaky to me, but I am not yet familiar enough with the codebase to quickly find out the reason.

django.db.utils.OperationalError: database table is locked: django_dramatiq_task

Yes, I think those tests have always been a little flaky so I wouldn't worry too much about them at the moment.

This commit itself will break test suite, so we can later address issue in #123.
This bug was introduced in #103 where django_dramatiq started benefitting from `AppConfig.ready()` functionality.
There was a research done in #100 explaining the underlying issue.
@Bogdanp Bogdanp merged commit fbe274d into Bogdanp:master Nov 5, 2022
@Bogdanp
Copy link
Owner

Bogdanp commented Nov 5, 2022

Looks good to me. Thanks again!

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.

2 participants