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

feat: add LRU cache for DID Documents #3651

Conversation

paullatzelsperger
Copy link
Member

What this PR changes/adds

  • Modifies the existing LruCache to become a ConcurrentLruCache.
  • uses the ConcurrentLruCache in the DidResolverRegistryImpl to avoid excessive network traffic.

Why it does that

Avoid excessive network traffic, when resolving the same did in close temporal vicinity.

Further notes

List other areas of code that have changed but are not necessarily linked to the main feature. This could be method
signature changes, package declarations, bugs that were encountered and were fixed inline, etc.

Linked Issue(s)

Closes #3645

Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.

@paullatzelsperger paullatzelsperger added this to the Milestone 13 milestone Nov 23, 2023
@paullatzelsperger paullatzelsperger added the enhancement New feature or request label Nov 23, 2023
@paullatzelsperger paullatzelsperger force-pushed the feat/3645_create_lru_cache_for_did_documents branch from 3cd2d84 to 8499a42 Compare November 23, 2023 17:10
@codecov-commenter
Copy link

codecov-commenter commented Nov 23, 2023

Codecov Report

Attention: 20 lines in your changes are missing coverage. Please review.

Comparison is base (2b5f079) 71.79% compared to head (8a60363) 71.74%.

Files Patch % Lines
...clipse/edc/util/collection/ConcurrentLruCache.java 36.36% 14 Missing ⚠️
...dc/iam/did/resolution/DidResolverRegistryImpl.java 66.66% 4 Missing and 2 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3651      +/-   ##
==========================================
- Coverage   71.79%   71.74%   -0.06%     
==========================================
  Files         915      915              
  Lines       18312    18342      +30     
  Branches     1038     1040       +2     
==========================================
+ Hits        13148    13159      +11     
- Misses       4710     4727      +17     
- Partials      454      456       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@paullatzelsperger paullatzelsperger force-pushed the feat/3645_create_lru_cache_for_did_documents branch 2 times, most recently from 9d97712 to e7440c1 Compare November 23, 2023 19:51
@paullatzelsperger paullatzelsperger force-pushed the feat/3645_create_lru_cache_for_did_documents branch from e7440c1 to 8a60363 Compare November 23, 2023 19:51
@paullatzelsperger paullatzelsperger merged commit 906bc17 into eclipse-edc:main Nov 23, 2023
17 checks passed
@paullatzelsperger paullatzelsperger deleted the feat/3645_create_lru_cache_for_did_documents branch November 23, 2023 20:58
@bscholtes1A
Copy link
Contributor

@paullatzelsperger sorry for the late review, but shouldn't we fetch again the DID document after some time? (even if it is present in the cache), in order to avoid to continue using an outdated version of the DID document?

@paullatzelsperger
Copy link
Member Author

paullatzelsperger commented Nov 25, 2023

@paullatzelsperger sorry for the late review, but shouldn't we fetch again the DID document after some time? (even if it is present in the cache), in order to avoid to continue using an outdated version of the DID document?

Yep, good point. Sounds like a task for a follow-up issue :)
I didn't bother with it now, because for our intents and purposes, the dids are rather long-living.
The intention here was to avoid multiple network calls in close temporal proximity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a LRU cache for resolved DidDocuments
4 participants