feat: Added SELECT redis command #482
Conversation
|
@apolukhin, any chance of being reviewed? |
|
Thanks for the ping! Will look at the PR in a few days |
There was a problem hiding this comment.
Looks good to me.
It's only worth adding a new option here
https://github.com/userver-framework/userver/blob/develop/samples/redis_service/tests/conftest.py#L17
to document it.
| @@ -27,16 +27,19 @@ struct ConnectionInfo { | |||
| bool read_only = false; | |||
| ConnectionSecurity connection_security = ConnectionSecurity::kNone; | |||
| using HostVector = std::vector<std::string>; | |||
| std::optional<size_t> database_index{}; | |||
There was a problem hiding this comment.
New connections always use database index 0 so it is not really an optional:
| std::optional<size_t> database_index{}; | |
| std::size_t database_index = 0; |
| @@ -267,6 +270,7 @@ class Redis::RedisImpl : public std::enable_shared_from_this<Redis::RedisImpl> { | |||
| uint16_t port_ = 0; | |||
| std::string server_; | |||
| Password password_{std::string()}; | |||
| std::optional<size_t> database_index_; | |||
There was a problem hiding this comment.
Not optional and 0 by default
| @@ -1099,6 +1106,47 @@ void Redis::RedisImpl::SendReadOnly() { | |||
| })); | |||
| } | |||
|
|
|||
| void Redis::RedisImpl::SelectDatabase() { | |||
| if (!database_index_) { | |||
There was a problem hiding this comment.
Better yet, if database index equals 0.
| if (settings.database_index && *settings.database_index == 0) { | ||
| // To get rid of the redundant `SELECT 0` command | ||
| // since 0 is the default database index and it will be set automatically | ||
| settings.database_index = std::nullopt; | ||
| } |
There was a problem hiding this comment.
Better check on connect in RedisImpl
| @@ -38,6 +39,14 @@ RedisMapSettings::RedisMapSettings(const formats::json::Value& doc) { | |||
| ? USERVER_NAMESPACE::redis::ConnectionSecurity::kTLS | |||
| : USERVER_NAMESPACE::redis::ConnectionSecurity::kNone; | |||
|
|
|||
| settings.database_index = | |||
| client_settings["database_index"].As<std::optional<size_t>>(); | |||
There was a problem hiding this comment.
Will not work in cluster mode, would be great if you add a check in redis/component.cpp
There was a problem hiding this comment.
Could you clarify this a little?
- Do you want me to extend method Redis::Connect with something like this:
auto sentinel = redis::Sentinel::CreateSentinel(
thread_pools_, settings, redis_group.config_name, config_source,
redis_group.db, redis::KeyShardFactory{redis_group.sharding_strategy},
cc, testsuite_redis_control);
if (sentinel) {
if (sentinel->IsInClusterMode() && settings.database_index != 0) {
// handle it somehow
}
...
}Is it the only one place I need to insert the check or there are some other places as well?
2. How do you want to handle it? Throw an exception or just skip it with the corresponding log message like when sentinel pointer turns out to be null?
3. Are you sure you want to add checks in this case? As you have mentioned, cluster setup does not support selecting databases, so service will shut down with an error message like SELECT failed: unknown command SELECT``anyway.
Added the ability to select a database for redis (see SELECT).
It seems to me that this feature will be useful to many, since people often use “communal” redis sentinel setup.
This can be done by adding the
database_indexkey to the redis config{ "redis_settings": { "taxi-tmp": { "password": "", "database_index": 3, /* <---- `SELECT 3` will be executed on connection. If 0 or no "database_index" key at all - noop */ "secure_connection": false, "shards": [ { "name": "mymaster" } ], "sentinels": [ { "host": "127.0.0.1", "port": 26379 } ] } } }