RESTDataSource cache: Support for redis sentinel

[eberhara]

Hey guys, As far as I know, apollo-server supports caching on redis, in memory or memcached. apollo-server-cache-redis uses npm's redismodule in order to create a redis client.

The redis module does not currently support redis sentinel, as per some issues like https://github.com/NodeRedis/node_redis/issues/302

I am opening this issue so we can discuss how to support redis sentinels on RESTDataSource caching.

So, should we create a new package like apollo-server-cache-redis-sentinel? Or, should we support sentinels on the existing apollo-server-cache-redis? Or, should the client using ApolloServer be responsible for creating a new KeyValueCache implementation that supports sentinels?

Thanks in advance!

[sbrichardson]

The easiest thing to do, if you need the feature now, would be to extend the class, overwriting the constructor of apollo-server-cache-redis, or just create a new package that does it directly etc.

You would just need to duplicate some code from the exisiting redis-cache package there.

I did this to use Google's Redis API with their client.

[eberhara]

Good idea.

I will probably extend the class on my server, but do you think it would be good if we had an “apollo-cache-redis-sentinel” as part of apollo packages?

[sbrichardson]

It's hard to say. I think if it was consistently implemented, other packages would be convenient. The current redis package is pretty basic and hard coded (not a bad thing).

It's a simple package and each implementation could be slightly different. For example, I use hashes hget vs the current mget.

With the current design, it would create a lot of duplicate code since everything in the constructor has to be copied, just to change the underlying client.

It could take an option with an already instantiated client like below, but then other methods may break, such as the close() or how they are promisfying methods, it would need to be fully compatible to work.

I think the original intent was to have an easy enough API for KeyValueCache that you just create a new cache package as needed.

export class RedisCache implements KeyValueCache {
   constructor(options: Redis.ClientOpts) {

    // Potentially could add an option to pass a client

    const { client: userClient, ...opts } = options

    const client = userClient || Redis.createClient(opts);

    // promisify client calls for convenience
    client.mget = promisify(client.mget).bind(client);
    client.set = promisify(client.set).bind(client);
    client.flushdb = promisify(client.flushdb).bind(client);
    client.quit = promisify(client.quit).bind(client);

    this.client = client;

    this.loader = new DataLoader(keys => this.client.mget(keys), {
      cache: false,
    });
  }

[eberhara]

Just created a PR #1770 with an implementation for that. Maybe we can discuss on top of that.

[abernix]

Happy to re-open if this isn't resolved with #1770, but I hope that did the trick! :)