-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Give each redshift client their own boto session #2766
Give each redshift client their own boto session #2766
Conversation
@jweibel22 Nice! I'd love to get this in for v0.18.1. Does this change work for you locally when you run with multiple threads? Also, could you:
|
079bd67
to
34388a6
Compare
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Github Build Bot.
|
since the boto session is not thread-safe, using the default session in a multi-threaded scenario will result in concurrency errors
34388a6
to
863d8e6
Compare
Yes, I have verified that is does resolve the issue when I run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very nice!
Could you add a test to the redshift unit tests in tests/unit/test_redshift_adapter.py
that makes sure we call boto3.Session().client()
instead of boto3.client()
directly in both the iam_profile cases (none and not none)? I would hate to regress on this.
Co-authored-by: Jacob Beck <beckjake@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This looks great to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this @jweibel22! I'll try to resolve the changelog conflict and get this in.
resolves #2756
Description
Since the boto session is not thread-safe, using the default session in a multi-threaded scenario will result in concurrency errors.
This change ensures that a fresh session is instantiated before creating the client. This is similar to what is happening in the else branch. Thus, this change should not cause any unexpected issues (e.g. exceeding service quotas)
Checklist
CHANGELOG.md
and added information about my change to the "dbt next" section.