-
Notifications
You must be signed in to change notification settings - Fork 24
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: cache boto3 swf clients #434
Conversation
Do not instantiate a swf client on each and every object creation. Use a per-thread cache as clients are not threadsafe. Signed-off-by: Yves Bastide <yves@botify.com>
We've got a generic cache architecture, let's use it for s3 too 🙂 Signed-off-by: Yves Bastide <yves@botify.com>
Signed-off-by: Yves Bastide <yves@botify.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.
Nice 👌
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.
Good idea, but let's use context vars
simpleflow/boto3_utils.py
Outdated
from __future__ import annotations | ||
|
||
import hashlib | ||
from threading import local |
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.
Please use ContextVars instead https://docs.python.org/3/library/contextvars.html
simpleflow/boto3_utils.py
Outdated
boto3_clients = _client_var.get("boto3_clients") | ||
if boto3_clients is None: |
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.
In [3]: _client_var = ContextVar("boto3_clients", default=None)
In [4]: boto3_clients = _client_var.get("boto3_clients")
In [5]: boto3_clients
Out[5]: 'boto3_clients'
By default, boto3_clients
is initialized to the string "boto3_clients"
* the "local data" variable must be global, obviously * replace threading.local with contextvars.ContextVar: the future's here! Signed-off-by: Yves Bastide <yves@botify.com>
ce81e03
to
e31bb92
Compare
simpleflow/boto3_utils.py
Outdated
key = hashlib.sha1(json_dumps(d).encode()).hexdigest() | ||
boto3_clients = _client_var.get() | ||
if boto3_clients is None: | ||
boto3_clients = {} |
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 line would then be removed
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.
Except my nitpick above, all good !
Good nitpicks, I'll apply them and make a rc1 🙂 |
Thank you Joachim! Co-authored-by: Joachim Jablon <ewjoachim@gmail.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.
a posteriori 👍
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.
yummy 😋
Signed-off-by: Yves Bastide yves@botify.com