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

make sure we reuse boto3 clients and resources #1528

Merged
merged 6 commits into from
Jan 17, 2025
Merged

Conversation

terrazoon
Copy link
Contributor

@terrazoon terrazoon commented Jan 8, 2025

Description

boto3 clients and resources are thread safe and have built in connection pooling, so we should reuse them as much as possible. Implement the singleton pattern to make sure we are doing so.

Security Considerations

N/A

@terrazoon terrazoon self-assigned this Jan 9, 2025
@terrazoon terrazoon linked an issue Jan 9, 2025 that may be closed by this pull request
@terrazoon terrazoon changed the title move s3 client and resource to init make sure we reuse boto3 clients and resources Jan 9, 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.

Thanks, @terrazoon! I took a look here and flagged a couple of things, and left a general comment/discussion question about the use of a global vs. just moving things into a class instead. We don't need to dig in deeply on that but I did want to call it out.

Thanks for all of the tests here, too! 🎉

app/aws/s3.py Outdated Show resolved Hide resolved
app/aws/s3.py Outdated Show resolved Hide resolved
notifications_utils/s3.py 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.

Thanks again, @terrazoon!

notifications_utils/s3.py Show resolved Hide resolved
@terrazoon terrazoon merged commit 687819a into main Jan 17, 2025
7 checks passed
@terrazoon terrazoon deleted the notify-api-1526 branch January 17, 2025 17:00
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.

Make sure we are always re-using boto3 clients
4 participants