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: cache boto3 swf clients #434

Merged
merged 5 commits into from
Oct 11, 2023

Conversation

ybastide
Copy link
Contributor

  • do not instantiate a swf client on each and every object creation. Use a per-thread cache as clients are not thread safe.
  • same for s3 clients

Signed-off-by: Yves Bastide yves@botify.com

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>
@ybastide ybastide added the bug label Oct 11, 2023
@ybastide ybastide requested a review from jbbarth October 11, 2023 13:19
Signed-off-by: Yves Bastide <yves@botify.com>
Copy link
Contributor

@SofienBenAyed SofienBenAyed left a comment

Choose a reason for hiding this comment

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

Nice 👌

Copy link
Contributor

@ewjoachim ewjoachim left a 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

from __future__ import annotations

import hashlib
from threading import local
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines 21 to 22
boto3_clients = _client_var.get("boto3_clients")
if boto3_clients is None:
Copy link
Contributor

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>
@ybastide ybastide force-pushed the reported-bug/DATA-18922/oom-on-simpleflow branch from ce81e03 to e31bb92 Compare October 11, 2023 15:07
simpleflow/boto3_utils.py Outdated Show resolved Hide resolved
simpleflow/boto3_utils.py Outdated Show resolved Hide resolved
simpleflow/boto3_utils.py Outdated Show resolved Hide resolved
key = hashlib.sha1(json_dumps(d).encode()).hexdigest()
boto3_clients = _client_var.get()
if boto3_clients is None:
boto3_clients = {}
Copy link
Contributor

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

Copy link
Contributor

@ewjoachim ewjoachim left a 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 !

@ybastide
Copy link
Contributor Author

Good nitpicks, I'll apply them and make a rc1 🙂
Thanks!

simpleflow/boto3_utils.py Outdated Show resolved Hide resolved
Thank you Joachim!

Co-authored-by: Joachim Jablon <ewjoachim@gmail.com>
@ybastide ybastide removed the request for review from jbbarth October 11, 2023 17:21
@ybastide ybastide merged commit 275062f into main Oct 11, 2023
7 checks passed
@ybastide ybastide deleted the reported-bug/DATA-18922/oom-on-simpleflow branch October 11, 2023 17:22
Copy link
Contributor

@ewjoachim ewjoachim left a comment

Choose a reason for hiding this comment

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

a posteriori 👍

Copy link
Collaborator

@jbbarth jbbarth left a comment

Choose a reason for hiding this comment

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

yummy 😋

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants