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

[multi-tenancy] add profile resource management #928

Conversation

TimoGlastra
Copy link
Contributor

This PR adds a very simple approach to multi-tenant profile management. A ProfileCache is added that has a max capacity (initialized with 100 for now) and keeps count of references using sys.getrefcount. It will close wallets when the capacity is exceeded and the profile is not used at the moment. The ledger implementation uses a custom refcounter using async context managers which may be nice to also use for this scenario. However this implementation allowed to build the cache on top of the profiles without modifying the profile itself.

await profile.close()

if len(self.profiles) <= self.capacity:
break
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems a bit odd that this will always iterate through the profiles, short-circuiting if there is one to be cleaned up and we are now below max capacity. Also self.profiles.items() will increase the refcount of the dict itself but not the entries it contains, as I understand it (and according to my test just now).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not that happy with this approach myself. I'll see if I can rework it a bit.

Do you have a suggestion on how to approach this? Do you think the way ledger handles refcounting will also work for here?

Base automatically changed from master to main February 2, 2021 12:58
@baegjae
Copy link
Contributor

baegjae commented Jun 23, 2021

@TimoGlastra I think this feature is essential when hosting multi-tenancy enabled aca-py services. Do you have any plans to complete it?

@TimoGlastra
Copy link
Contributor Author

Yes definitely, good you bring it up. Just need to revisit the implementation and resolve the comment from Andrew

@dbluhm
Copy link
Contributor

dbluhm commented Jun 30, 2022

Closing this PR as superseded by #1692

@dbluhm dbluhm closed this Jun 30, 2022
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.

4 participants