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

refactor(cache)!: change default cache to in memory #1389

Conversation

TimoGlastra
Copy link
Contributor

Changes default cache implementation to an in memory cache. This simplifies the caching, and makes sure it works for all cases (nodejs/react-native, but also mulit-tenancy).

BREAKING CHANGE:
The default cache implementation has been changed from SingleContextStorageLruCache to InMemoryLruCache. In React Native environments it is still recommended to use the SingleContextStorageLruCache cache implementation, but due to it's dependence on the wallet storage, it is not recommend for server side or multi-tenancy deployments. A custom caching interface can be implemented in those cases.

@TimoGlastra TimoGlastra requested a review from a team as a code owner March 18, 2023 22:46
@TimoGlastra TimoGlastra changed the title refactor!(cache): change default cache to in memory refactor(cache)!: change default cache to in memory Mar 18, 2023
@TimoGlastra TimoGlastra force-pushed the refactor/change-default-cache branch from e578fc4 to 493595a Compare March 18, 2023 22:48
@codecov-commenter
Copy link

codecov-commenter commented Mar 18, 2023

Codecov Report

Merging #1389 (9ece062) into main (343ce6a) will increase coverage by 3.49%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1389      +/-   ##
==========================================
+ Coverage   81.82%   85.32%   +3.49%     
==========================================
  Files         788      788              
  Lines       19405    19406       +1     
  Branches     3150     3150              
==========================================
+ Hits        15879    16559     +680     
+ Misses       3519     2840     -679     
  Partials        7        7              
Impacted Files Coverage Δ
packages/core/src/modules/cache/CacheModule.ts 100.00% <100.00%> (ø)

... and 14 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@genaris
Copy link
Contributor

genaris commented Mar 20, 2023

I don't know the details about it but an in memory cache makes me think that, unless a server has 100% of uptime, it will be lost. Would that be something that we should leverage using by default?

I totally agree that it's still better than SingleContextStorageLruCache for multi-tenancy and probably also for server-side single-tenancy agents, but given that AFJ is mostly used in Mobile Agents where, as you said, it's preferable to use the other one, I'm not convinced that in memory should be set to default in 0.4.0.

The main concern about SingleContextStorageLruCache is that it does not work in multi-tenancy, right? In such case couldn't be an error thrown at agent initialization when attempting to use it for multi-tenancy?

As I said previously, I don't know so much about the context so it's very likely that my reasoning is wrong, but that's what I'm thinking based on some things that we recently defaulted to the 'right way' and caused some troubles to current AFJ audience (e.g. setting the did:key flag to true in 0.3.0).

@TimoGlastra
Copy link
Contributor Author

I don't know the details about it but an in memory cache makes me think that, unless a server has 100% of uptime, it will be lost. Would that be something that we should leverage using by default?

I totally agree that it's still better than SingleContextStorageLruCache for multi-tenancy and probably also for server-side single-tenancy agents, but given that AFJ is mostly used in Mobile Agents where, as you said, it's preferable to use the other one, I'm not convinced that in memory should be set to default in 0.4.0.

The main concern about SingleContextStorageLruCache is that it does not work in multi-tenancy, right? In such case couldn't be an error thrown at agent initialization when attempting to use it for multi-tenancy?

As I said previously, I don't know so much about the context so it's very likely that my reasoning is wrong, but that's what I'm thinking based on some things that we recently defaulted to the 'right way' and caused some troubles to current AFJ audience (e.g. setting the did:key flag to true in 0.3.0).

Yeah good point. Maybe we can automatically set the cache implementation based on environment / setup. I think for server implementations it's fine to store the cache in memory and promote users to provide their own caching implementation (or we could add default implementations for e.g. redis). But for RN we probably don't want to use the in memory cache. 👍

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.

Move away from SingleContextStorageLruCache as default cache implementation
3 participants