Skip to content

[BUG] FailoverClusterClient ignore DB option #3339

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

Closed
khasanovbi opened this issue Apr 8, 2025 · 8 comments
Closed

[BUG] FailoverClusterClient ignore DB option #3339

khasanovbi opened this issue Apr 8, 2025 · 8 comments
Assignees
Labels

Comments

@khasanovbi
Copy link
Contributor

khasanovbi commented Apr 8, 2025

FailoverClusterClient ignore DB from config

Expected Behavior

FailoverClusterClient use DB from FailoverOptions

Current Behavior

FailoverClusterClient use default DB: 0 option

@ndyakov
Copy link
Member

ndyakov commented Apr 10, 2025

@khasanovbi thank you for reporting, as you can see from

// Database to be selected after connecting to the server.
, the DB option is available only for single-node clients (and single node failover client). It is not available for Cluster Clients.

@khasanovbi
Copy link
Contributor Author

Yes, but this is a valid Redis configuration.
I'm using Redis Sentinel.
Previously I was using FailoverClient with DB: 1, but wanted to go not only to master but also to replicas with RouteByLatency.
To do this I switched to using FailoverClusterClient, I realize it is a pseudo cluster client but it should be able to go to DB:1.

@ndyakov
Copy link
Member

ndyakov commented Apr 10, 2025

@khasanovbi I understand your use case, but under the hood the FailoverClusterClient is using the ClusterClient. The SELECT command for selecting a database is not enabled in cluster mode. Although I am against adding the DB option for the cluster client, I think there is a good workaround you can utilize. You should be able to use

OnConnect func(ctx context.Context, cn *Conn) error
and execute cn.Select yourself:

....
    OnConnect: func(ctx context.Context, cn *redis.Conn) error {
        _, err := cn.Select(ctx, 1)
        return err
    },
....

Feel free to close this if you are satisfied with the workaround.

@ndyakov ndyakov self-assigned this Apr 10, 2025
@khasanovbi
Copy link
Contributor Author

Ok, thank you for workaround, perhaps this should then be specified in the documentation?

@khasanovbi
Copy link
Contributor Author

khasanovbi commented Apr 10, 2025

I tried such workaround, but it try to send select command to sentinel, instead of redis instance.

sentinel: GetMasterAddrByName master=\"test\" failed: ERR unknown command 'select', with args beginning with: '1' "

Maybe we could add SentinelDialer, OnSentinelConnect fields to FailoverOptions to distinguish such connections

@ndyakov
Copy link
Member

ndyakov commented Apr 10, 2025

Hm, correct, the sentinel client will inherit the same callback. I assume you can simply check if the err is the unknown command err and ignore it. Any other error should make sense for your application.

@ndyakov
Copy link
Member

ndyakov commented Apr 14, 2025

@khasanovbi I will close this issue for now, feel free to reopen if the solution doesn't work for you and you would like to discuss a different approach.

@khasanovbi
Copy link
Contributor Author

khasanovbi commented Apr 15, 2025

Hi @ndyakov
I don't really like this solution as we are making an extra call, and we also have to catch an error by message.
I would suggest moving the db select to the client constructor and prepare some small PR #3342, PTAL.

PS. I'm don't have permissions to reopen issue)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants