Skip to content
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

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

Open
mehdipourfar opened this issue Mar 4, 2024 · 10 comments
Open
Labels
feature good first issue Good for newcomers

Comments

@mehdipourfar
Copy link

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.

@rueian
Copy link
Collaborator

rueian commented Mar 4, 2024

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.

@mehdipourfar
Copy link
Author

mehdipourfar commented Mar 4, 2024

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

@rueian
Copy link
Collaborator

rueian commented Mar 4, 2024

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

@rueian
Copy link
Collaborator

rueian commented Mar 4, 2024

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.

@mehdipourfar
Copy link
Author

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

@rueian rueian added good first issue Good for newcomers feature labels Mar 6, 2024
@erdemtuna
Copy link
Contributor

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

@rueian
Copy link
Collaborator

rueian commented Mar 11, 2024

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.

@erdemtuna
Copy link
Contributor

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

@erdemtuna
Copy link
Contributor

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.

5053565

@rueian
Copy link
Collaborator

rueian commented Mar 23, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants