-
Notifications
You must be signed in to change notification settings - Fork 124
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
Support for Redis Sentinel and Cluster #77
Comments
Adding to the discussion the current node-redis roadmap redis/node-redis#1040 |
BTW, @amitshlo Will gladly merge a PR. |
Like you saw, I started working on it. Any thoughts (and, what do you think about the fact that people will need 2 clients to send)? |
I understand completely the reason and also I would have done it this way in order to avoid managing the clients connections on this package, but I would like to have a default client being created for you as this is two connections. My plan would be to create optional fields to provide the publish and subscribe connection by the user, create optional dependency on redisio and if the user has not provided subscribe or publish clients then I will create a default one (check that the dependency is satisfied). I would like to have this logic in order to allow users fast prototyping as subscriptions could be a bit of a hassle to setup at the moment. What do you think of this plan? |
Sounds good, actually it's also done in many packages of this kind (session stores for example). |
I mean npm's optional https://docs.npmjs.com/files/package.json#optionaldependencies |
OK, very cool! |
In my opinion both option 1 and 2 should be implemented at some point. Being able to pass in subscriber and publisher client would mean the library consumer has direct and explicit control of the redis streams. Regardless of whether option 1 gets implemented though I do think that |
@amitshlo any status on the PR you are working on? I would also happily contribute to implementing one of the above options. |
@julienvincent it's actually almost complete, there's one comment @davidyaha gave me about tests that I don't understand, so I'm waiting for him to reply... |
Add support for Redis Sentinel and Cluster.
In production most places use Sentinel or Cluster for high availability and automatic failovers.
The library now used by 'graphql-redis-subscriptions' isn't suppotring it.
Suggested solutions:
I personally think the second one is better, because it's not a breaking change and there's not much use in letting the developers send a redis instance.
Maybe I will do 2 and open a PR in the near future.
The text was updated successfully, but these errors were encountered: