novuhq / novu

Open-Source Notification Platform. Embeddable Notification Center, E-mail, Push and Slack Integrations.
https://novu.co
Other
34.11k stars 3.48k forks source link

[NV-1545] šŸš€ Feature: Accept Redis cluster (Sentinel) in Redis config #2523

Open MarekSalgovic opened 1 year ago

MarekSalgovic commented 1 year ago

šŸ”– Feature description

Add environment variables, so Redis client can be used with sentinels in configuration.

šŸŽ¤ Why is this feature needed ?

We use Redis cluster and we would like to connect Novu to it. We have multiple hosts while Novu only supports single host address in REDIS_HOST environment variable. Our connection string: redis+sentinel://redis-0,redis-1,redis-2/master/15

āœŒļø How do you aim to achieve this?

As I was reading through the code, both ioredis and bull support this feature. I would add 2 new env vars:

and rework how Redis/Bull client is initialized

alternatively, a more sophisticated solution would be to add REDIS_URI env variable that parses and overrides other redis configuration - host, port, db_index... REDIS_URI: redis+sentinel://redis-0:2000,redis-1:3000,redis-2:4000/master/15

šŸ”„ļø Additional Information

I found this issue in the Bull repo which confirms this is achievable: https://github.com/OptimalBits/bull/issues/396.

šŸ‘€ Have you spent some time to check if this feature request has been raised before?

šŸ¢ Have you read the Code of Conduct?

Are you willing to submit PR?

None

NV-1545

oba2311 commented 1 year ago

@scopsy could help with this one?

scopsy commented 1 year ago

@oba2311 I think that @MarekSalgovic outlined this very good in the issue, I've added the help wanted for this one. In case someone wants to help with it.

aggmoulik commented 1 year ago

Hey @scopsy I can help with it, can you let me know which approach would be good ? URI or adding two new environment variables.

I recommend to use URI approach.

jainpawan21 commented 1 year ago

@aggmoulik Assigning to you. I also think URI approach is better.