mcollina / async-cache-dedupe

Async cache with dedupe support
MIT License
641 stars 40 forks source link

support @upstash/redis client #41

Closed cemreinanc closed 1 year ago

cemreinanc commented 1 year ago

Hello,

I wanted to try to add support for @upstash/redis http client for using this package on serverless functions on edge. current work in progress can be viewed here: https://github.com/cemreinanc/async-cache-dedupe/tree/feature/upstash

I copied and modified redis storage code and tests to adapt with upstash sdk, but the problem is upstash http client connects to a replica set and it only supports eventual consistency and not strong consistency where we can read our writes immediately. Thats why the half of the test suite fails with the new upstash storage.

Now I have couple of questions:

And finally do you think this support is achievable under this circumstances?

mcollina commented 1 year ago

Why would you use the REST API instead of just using the Redis protocol? I think that should work out of the box: https://docs.upstash.com/redis/features/consistency.

read-after-write consistency is currently needed for this to work, otherwise multiple concurrent misses will result in a storm of calls to upstream.

Using a "full featured" local cache would result on having the various caches out of sync, which is something you really want to avoid. Something that could be done is a cache with a low TTL to overcome this limitation of Upstash (1-5secs).

A better production solution to this is to implement stale-while-revalidate (https://github.com/mcollina/async-cache-dedupe/issues/33). In this scenario the cache would "never" miss on a hot data path.

simone-sanfratello commented 1 year ago

see https://github.com/mcollina/async-cache-dedupe/pull/45

cemreinanc commented 1 year ago

Actually the option to use REST API over default Redis protocol enables this library to be usable in Edge runtimes like (Vercel Edge, Cloudflare Workers etc.), which has much lower latency and no cold starts compared to classic serverless functions. https://vercel.com/docs/concepts/functions/edge-functions#why-use-edge-functions https://upstash.com/blog/vercel-edge

the new stale option looks like it might help but as it looks like it's decoupled from the storage adapters itself there is still a need to create a new storage adapter with a little bit different mindset from the classic redis option. It needs to utilize the swr by default and probably need to force it to work correctly. Which still got me confused about what to do about implementation.

Also I understand that might not be a priority or implementation could be tricky under the current state of the library. (And actually it's not a must but nice to have because like you said it can work with default protocol and http option is a further optimization in this edge case. And I tried a quick implementation in the first post thinking that might be an easy addition but currently looks like it's not)

Anyways I would like to thank you all for the great work that you have done in this and many other OSS projects.

mcollina commented 1 year ago

Unfortunately neither @simone-sanfratello or myself are users of Upstash, so this is unlikely to happen without a significant contribution.

As things looks right now, writing a storage adapter that requires stale to be set should work.

jokull commented 1 year ago

@cemreinanc Although this library looks awesome, it seems to be geared towards a long-running-process use case, wherein multiple requests are accessing a single client instance. In the edge environment you would be spinning up a new client for each request - and probably lose out on some of the features here like stale-while-revalidate. It's a different paradigm. Let me know if you find a good edge-paradigm library for this though! I'm evaluating my choices.

mcollina commented 1 year ago

Closing for the above reason ^.