rueidis icon indicating copy to clipboard operation
rueidis copied to clipboard

feature request: adding an option to disable dial on creating client

Open mehdipourfar opened this issue 1 year ago • 11 comments

When we initialize a redis client with go-redis, it does not return any error. I think this is a very good feature. Imagine I use redis in my web service only as a cache. If it goes down, it is still ok and I can use the main repository for fetching my data. If it goes up during the service runtime, I will use that cache again. But this package returns nil and error if it cannot connect to redis on initialization and I don't want to interrupt my service start up because my redis server is out of reach.

mehdipourfar avatar Mar 04 '24 13:03 mehdipourfar

Hi @mehdipourfar,

If redis servers go down after the initialization, rueidis will try reconnection automatically and can work as you want.

rueidis has a good reason to return an error at initialization, allowing users to verify whether their config for establishing a Redis connection is correct or not. I generally recommend users to fail fast and fix fast.

Indeed, this approach will interrupt service startup if their config is correct but Redis server are not ready. However, there is another purpose of initialization which I think is the true blocker to your request: choosing the correct client implementation.

Currently, rueidis will connect to the server to check if it is a Redis cluster. If yes, then the initialization returns a cluster client implementation, otherwise returns a standalone client implementation.

If we let this behavior be configurable, then we can return a concrete client implementation instead of a nil even when the initialization has failed.

rueian avatar Mar 04 '24 14:03 rueian

Thanks for your reply. What happens if we do not try to Dial in newSentinelClient and newSingleClient?

mehdipourfar avatar Mar 04 '24 14:03 mehdipourfar

You will have no chance to know whether your config is wrong or not before sending your first redis request.

rueian avatar Mar 04 '24 15:03 rueian

I think we should keep the Dail() even if we make the auto-detection configurable. Users can just ignore the error returned by the initialization if they don't care about it.

rueian avatar Mar 04 '24 15:03 rueian

That would solve my problem if we don't return nil as client.

mehdipourfar avatar Mar 06 '24 10:03 mehdipourfar

Hey @rueian, can we clarify the description and expected change of this task for a potential implementation?

erdemtuna avatar Mar 10 '24 13:03 erdemtuna

Hi @erdemtuna,

The goal is to make the rueidis.NewClient return a concrete client implementation even if there is an error occurs. However, which concrete client implementation to use is currently queried from an online redis instance. This is the first blocker that we are unable to return a concrete implementation. We need a method to let users specify which implementation to use explicitly.

Since we already have the option ForceSingleClient, a potential implementation starts from adding a new one ForceClusterClient:

  1. When ForceClusterClient is true, let rueidis.NewClient return a clusterClient even if there is an error occurs.
  2. When ForceSingleClient is true, let rueidis.NewClient return a singleClient even if there is an error occurs.
  3. When Sentinel.MasterSet is set, let rueidis.NewClient return a sentinelClient even if there is an error occurs.
  4. Make sure all the above clients can work after redis is back.

rueian avatar Mar 11 '24 15:03 rueian

Thanks @rueian, I will work on the issue and communicate wherever necessary.

erdemtuna avatar Mar 11 '24 20:03 erdemtuna

Hi @rueian, I came up with these edits as a basic try however, I cannot make them work in the following scenario:

  • Create a client when the cluster is down.
  • Start the cluster.
  • Run a redis command with the client.

I seem to be stuck as of now. If someone could guide me, I would appreciate it. Otherwise, I should stop working on the issue.

https://github.com/redis/rueidis/commit/5053565ef72de657d0d945bf33e19805ec553d06

erdemtuna avatar Mar 17 '24 18:03 erdemtuna

Hi @erdemtuna, could you elaborate more on how you are being stuck?

rueian avatar Mar 23 '24 12:03 rueian

@erdemtuna Would you like to continue on this otherwise I can pick it up // @rueian

SoulPancake avatar Jun 28 '24 17:06 SoulPancake