Skip to content

Deprecation: improve Celery task db query #10414

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

Merged
merged 5 commits into from
Jun 12, 2023
Merged

Conversation

humitos
Copy link
Member

@humitos humitos commented Jun 8, 2023

This commit excludes the users that already have an unread notification with this message. This is only to avoid performing db queries for users where our notification backend won't add a new notification anyways. This will improve the performance of this task making it to run faster (hopefully).

This commit excludes the users that already have an unread notification with
this message. This is only to avoid performing db queries for users where our
notification backend won't add a new notification anyways. This will improve the
performance of this task making it to run faster (hopefully).
@humitos humitos requested a review from a team as a code owner June 8, 2023 22:00
@humitos humitos requested a review from benjaoming June 8, 2023 22:00
@humitos
Copy link
Member Author

humitos commented Jun 8, 2023

I found the slow query. It's because it passes 85k slugs into the query:

user_projects = (
AdminPermission.projects(user, admin=True)
.filter(slug__in=projects)
.only("slug")
)

humitos added 4 commits June 9, 2023 01:22
Instead of passing 82k slugs on each query to get the user's projects,
we perform a set() intersection in Python which is fast.

This reduces the time for this query in a pretty noticeable way.
This will interfere with the logic for sending email notifications. We do want
to send an email even if the user didn't read the onsite notification.
@humitos
Copy link
Member Author

humitos commented Jun 9, 2023

I tried this code in production and it's a lot faster than the previous implementation. Using Python set() is way better than dumping 82k slugs into the SQL query. So, I'm to merge this PR 👍🏼

Copy link
Contributor

@benjaoming benjaoming left a comment

Choose a reason for hiding this comment

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

Makes sense 👍

Definitely okay to have a slow query for this case, but I totally understand if it got too slow. And that query does seem very slow :)

The code is also pretty readable, and it's definitely preferable to have correctness over performance :)

@humitos humitos merged commit 098fc34 into main Jun 12, 2023
@humitos humitos deleted the humitos/deprecation-task-query branch June 12, 2023 08:42
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