feat: improve caching - adds TTL, cacheTransport and cacheGetKey #739
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Ilya Amelevich <ilya.amelevich@gmail.com>
Pull Request Test Coverage Report for Build 5069068464
💛 - Coveralls |
|
What about performance before and after these changes? |
I'm not sure are there exists any preformance tests to measure it. Probably you can help me with that? The best approach for caching - let user define cache logic and transport for caching. Just provide interface. I will try to do that when will have some free time. Current solution just make built-in caching more useful, cause for a lot of usage scenarios it's not enough to cache only first response. |
refactor: move default cache implementation to the separate file Signed-off-by: Ilya Amelevich <ilya.amelevich@gmail.com>
Signed-off-by: Ilya Amelevich <ilya.amelevich@gmail.com>
lance
reviewed
May 17, 2023
There was a problem hiding this comment.
@iamelevich this looks great! Thanks for the contribution. 🙇 I just have one small suggestion.
Signed-off-by: Ilya Amelevich <ilya.amelevich@gmail.com>
|
@lance Done. Please check. |
lance
changed the title
lance
approved these changes
May 24, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Current cache implementation not really useful in production, because it can store only first response, but this behaviour not good for a lot of cases. For example: I'm using Circuit Breaker to get config data and endpoint returns different responses based on arguments. I want it to be cached, but based on parameters.
Solution
options.cacheTransportyou can implement cache solution that you want.cacheTTLoption to update cached value after expiration time.cacheGetKeyoption to implement way how to get cache key from arguments.Problems
cacheTransportshould be sync, so you can only implement something that don't need async operations.JSON.stringifyand this is not optimal solution, so better to customize it.