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

Redis standalone #822

Open
wants to merge 53 commits into
base: develop
Choose a base branch
from

Conversation

ae-ignatiev
Copy link

@ae-ignatiev ae-ignatiev commented Jan 7, 2025

Redis standalone implementation.

  • New sharding_strategy RedisStandalone
  • New implementation class StandaloneImpl of interface SentinelImplBase. Based on cluster topology implementation without nodes discovery and single ClusterShard shard instance only.
  • Subscribers adopted to construct correct implementation
  • Added pytest plugin redis_standalone to run single redis instance. Plugin use the same approach and configs as in ya testsuite plugins to run redis and redis cluster: https://github.com/yandex/yandex-taxi-testsuite/tree/develop/testsuite/databases/redis
  • Functional test pubsub
  • Functional test integration_tests that was not used. Use testcase there to run standalone version. Tests for cluster fail there, marked them as failing cases for now.

Note: by creating a PR or an issue you automatically agree to the CLA. See CONTRIBUTING.md. Feel free to remove this note, the agreement holds.

@ae-ignatiev ae-ignatiev marked this pull request as ready for review January 9, 2025 07:53
@ae-ignatiev ae-ignatiev requested a review from segoon as a code owner January 9, 2025 07:53
std::vector<SentinelCommand> waiting_commands;

{
const std::lock_guard<std::mutex> lock(command_mutex_);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apolukhin А это тот код редиса, который на стандартных потоках, и тут нельзя использовать юсерверные мьютексы?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

да

@v-for-vandal
Copy link
Contributor

@ae-ignatiev Hi! Could you please give some motivation for this changes, so it would be easier for (some of) us to understand what and why?
(I don't know what your prefered language is, so I write in english, just in case).

  • What are real world use cases for this type of redis installation ?
  • What is the primary difference with current non-cluster topologies, that lead to new sharding strategy ? Can any current 'cluster/non-cluster' implementation work with standalone shard, may be with some minor modifications ?
  • The standalone_impl.cpp is large. Is it all unique ? Anything that can be moved to some common files and shared with other sharding strategies ?

@ae-ignatiev
Copy link
Author

@v-for-vandal , hi

  1. Existing solutions (either sentinel or cluster) seem to be too much over killing in simple and local scenarios. For example using single instance of redis as a local storage/local notifications/cache shared among several services. Standalone mode already has all required functionality without additional nodes. There are no critical requirements for resilience and reliability, no distribution in some solutions. And to be honest the main motivation is not to deploy the amount of redis instances that are required in cluster and sentinel modes. As far as I understood:

    Plus this single mode is good starting point to work with redis in the userver. Further in case of performance or other issues it is easy to migrate to cluster or smth like this.

  2. As I saw sharding strategy controls the impl is creating inside Sentinel class: https://github.com/userver-framework/userver/blob/develop/redis/src/storages/redis/impl/sentinel.cpp#L89, not directly but anyway. And here the problem is not in a sharding strategy but in commands that are used in ClusterSentinelImpl and SentinelImpl. The simplest redis installation do not know commands that ClusterSentinelImpl and SentinelImpl send to it (like SENTINEL MASTERS, cluster nodes, cluster slots). Probably some minor changes are possible (for instances in ClusterSentinelImpl) but it seems to me it becomes confusing to use one of existing implementation with some kind of flags for standalone or smth like that.
    So additional sharding strategy RedisStandalone contains dummy implementation and used mostly to choose correct impl.

  3. Yea, standalone_impl.cpp is large but almost all methods there are copypasted from cluster_sentinel_impl.cpp and cluster_topology.cpp. So there is not all unique. The goal was not to impact existing and already working code. That's why I didn't try to designate and move shared code. And, to be honest, it's quite complex task to implement.

And yes, we can use Russian if it is ok here)

@VyacheslavVanin
Copy link

I think it is better to implement not an SentinelImplBase derivative class but make some
TopologyHolderBase by extracting interface of existing ClusterTolopogyHolder,
because ClusterTopologyHolder is the class responsible for storing and updating topology, and seems like it is what you want to customize.
It will also make it easier to reimplement later Sentinel-related code and get rid of old SentinelImpl code.
And then implement StandaloneTopologyHolder that always returns the same topology of single shard and of single instance from this method.

There is usage of ClusterTopologyHolder.
here we can choose implementation depending on key_shard

@ae-ignatiev
Copy link
Author

@VyacheslavVanin , hi!

Could you please take a look at updates? Is that approach you've mentioned?
There are standalone_topology_holder.{hpp,cpp} and topology_holder_base.hpp.

There is a doubt regarding method implementation SetConnectionInfo. As I understood calling this method and changing the connection params causes updating the nodes set and topology.

Do not know whether it is applicable for a single node configuration or not. With empty method SetConnectionInfo implementation is easier but nodes configuration is completely static and can't be changed in runtime.

std::tie(conn_to_create_.host, conn_to_create_.port) = new_conn.HostPort();
conn_to_create_.connection_security = new_conn.GetConnectionSecurity();
conn_to_create_.read_only = new_conn.IsReadOnly();
// conn_to_create_.password = ???
Copy link

@VyacheslavVanin VyacheslavVanin Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change type of conn_to_create_ to ConnectionInfoInt and just copy new_conn there?
It seems like there will be less conversions.

We will update password later. Let it be as is

@VyacheslavVanin
Copy link

VyacheslavVanin commented Jan 21, 2025

@VyacheslavVanin , hi!

There is a doubt regarding method implementation SetConnectionInfo. As I understood calling this method and changing the connection params causes updating the nodes set and topology.

Do not know whether it is applicable for a single node configuration or not. With empty method SetConnectionInfo implementation is easier but nodes configuration is completely static and can't be changed in runtime.

@ae-ignatiev, hi!

This method is called when secdist updates. And secdist contains hosts, ports and password, so it is applicable.

I think we can leave password unchanged in SetConnectionInfo. We will fix this later

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

Successfully merging this pull request may close these issues.

3 participants