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 notification insert retry logic #1450

Merged
merged 2 commits into from
Dec 2, 2024
Merged

fix notification insert retry logic #1450

merged 2 commits into from
Dec 2, 2024

Conversation

terrazoon
Copy link
Contributor

@terrazoon terrazoon commented Dec 2, 2024

Description

There was flaw in the logic that inserts notifications into the db. In the event of a duplicate insert, we were supposed to quit trying immediately. However, because IntegrityError is a subclass of SQLAlchemyError we ended falling through the catch phase and activating the retry logic. So we ended up getting 5 copies of the duplicate error in the logs, and because we hit the SQLAlchemyError the whole exception stack trace got printed, which resulted in millions of log lines when we ran a 25k send.

Fix the logic so we exit abruptly on a duplicate insert. This short circuits the retry logic and results in only one warning level log line. This, in combination with having each worker support 500 tasks instead of 200 tasks, should reduce the volume of logging from retries by over 90%. Note that there is a separate category of log pollution for 'NoSuchKey' that is written up in a separate ticket and not directly related to this issue (although eliminating the retries may end up helping).

Security Considerations

N/A

@terrazoon terrazoon changed the title fix logic fix notification insert retry logic Dec 2, 2024
@terrazoon terrazoon self-assigned this Dec 2, 2024
Copy link
Member

@A-Shumway42 A-Shumway42 left a comment

Choose a reason for hiding this comment

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

Great catch! Looks good.

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.

Thanks, @terrazoon!

@ccostino ccostino merged commit 7a90722 into main Dec 2, 2024
7 checks passed
@ccostino ccostino deleted the notify-api-1430 branch December 2, 2024 21:38
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 the sqlalchemy.exc.IntegrityError exceptions from large batch send test and report findings
4 participants