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

Investigate bulk_save_objects for notification inserts #1533

Merged
merged 35 commits into from
Jan 15, 2025
Merged

Conversation

terrazoon
Copy link
Contributor

@terrazoon terrazoon commented Jan 10, 2025

Description

The next roadblock on scaling up to millions of notifications a day is the fact that we current insert notifications to the db one at a time. Try batch inserts using redis, a scheduled task, and bulk_save_objects().

So instead of writing one at a time to the db, and immediately sending to AWS afterwards, we:

  1. write to redis
  2. after writing to redis, send the notifications to AWS on a delay (60 seconds). It's on a delay so there is enough time to get the notifications written to the db before we need to process the sending.
  3. every 10 seconds, grab notifications from redis and batch write to the db

Security Considerations

With this new approach, notifications would held in redis for 10-50 seconds, meaning potentially there could be a few hundred notifications in redis at any particular time when big sends are happening.

@terrazoon terrazoon marked this pull request as draft January 10, 2025 15:59
@terrazoon terrazoon marked this pull request as ready for review January 13, 2025 21:22
@terrazoon terrazoon changed the title try batch inserts Investigate bulk_save_objects for notification inserts Jan 13, 2025
Copy link
Contributor

@ccostino ccostino left a comment

Choose a reason for hiding this comment

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

Awesome, thanks @terrazoon!

There were some general questions I had, and this is just for the saves/inserts - not any updates, right?

Thanks for writing/updating all of the tests, too, those look good!

app/celery/scheduled_tasks.py Show resolved Hide resolved
app/celery/scheduled_tasks.py Show resolved Hide resolved
app/celery/scheduled_tasks.py Show resolved Hide resolved
app/models.py Outdated Show resolved Hide resolved
notifications_utils/clients/redis/redis_client.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ccostino ccostino left a comment

Choose a reason for hiding this comment

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

Awesome, everything looks good to me now @terrazoon, thank you!

@terrazoon terrazoon self-assigned this Jan 15, 2025
@terrazoon terrazoon linked an issue Jan 15, 2025 that may be closed by this pull request
@terrazoon terrazoon merged commit 0a7ccc5 into main Jan 15, 2025
7 checks passed
@terrazoon terrazoon deleted the notify-api-1531 branch January 15, 2025 17:47
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.

Investigate bulk_save_objects for notification inserts
3 participants