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

Reads from persistent cache are not retained in memory cache #22

Open
sammacbeth opened this issue Sep 7, 2019 · 2 comments
Open

Reads from persistent cache are not retained in memory cache #22

sammacbeth opened this issue Sep 7, 2019 · 2 comments

Comments

@sammacbeth
Copy link
Contributor

When providing a persistentCache option, the cache is only read if there is a memory cache miss, and the lookup fails. In this case, the result from the persistent cache is not saved to the memory cache. When this happens, it is likely to be a repeated failure (e.g. device is offline), so the persistentCache will be repeatedly hit for every dns request. As the cache is persistent, it is likely to be much slower to retrieve results. Maybe it makes sense for the persistentCache result to be saved in the memory cache?

Perhaps it would also make sense to consult the persistent cache directly after a cache miss, to allow lookups without hitting the network after an app restart, for example. This would likely require the cache to also return a ttl though.

@pfrazee
Copy link
Contributor

pfrazee commented Sep 9, 2019

+1 on both suggestions: saving the p-cache read to the m-cache, and storing the ttl to the p-cache to allow for cached reads after restart. @sammacbeth do you have time to PR those updates?

@sammacbeth
Copy link
Contributor Author

Sure, I'll try have a look this week.

I'm wondering how this could be added without causing a breaking change. At the moment read should return a string or throw an error. This gives no space to add an extra ttl value. One options would be to just assume a short ttl (say a few minutes) for the m-cache. This would prevent the p-cache getting hammered, but not require a change to it's API shape.

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

No branches or pull requests

2 participants