More configurable Redis Cluster

[Agalin]

Problem Statement

When configuring Redis Cluster in settings only a few selected options are supported (startup_nodes and readonly_mode + a few more hardcoded values).

Not only it differs from Redis Blaster clusters (where whole config is passed after a minor transformation to hosts) but also blocks some important options:

• Host/Port remapping.
• User and password for authentication (or at least I haven't found a way to pass those as parts of the host name).
• Read from replica (not sure if Sentry would work correctly in this mode).

Solution Brainstorm

Currently I'm using a small reimplementation of _RedisCluster's factory method that passes a copy of the whole config excluding is_redis_cluster (custom Sentry parameter) and hosts as it's overwritten explicitly):

def custom_factory(self, **config):
    hosts = config.get("hosts")
    hosts = list(hosts.values()) if isinstance(hosts, dict) else hosts

    def cluster_factory():
        if config.get("is_redis_cluster", False):
            # Copy config and remove Sentry-specific keys.
            safe_config = deepcopy(config)
            del safe_config["is_redis_cluster"]
            hosts = safe_config.pop("hosts")
            return RetryingRedisCluster(
                startup_nodes=hosts,
                decode_responses=True,
                skip_full_coverage_check=True,
                max_connections=16,
                max_connections_per_node=True,
                **safe_config  # Pass all options to the Redis Cluster client.
            )
        else:
            host = hosts[0].copy()
            host["decode_responses"] = True
            return (
                import_string(config["client_class"])
                if "client_class" in config
                else StrictRedis
            )(**host)

    return SimpleLazyObject(cluster_factory)

It works fine but I assume there are reasons only selected list of options is supported.

[getsentry-release]

Routing to @getsentry/team-web-sdk-backend for triage. ⏲️

[getsentry-release]

Routing to @getsentry/app-backend for triage. ⏲️

[untitaker]

@Agalin feel free to submit any improvements to consistency across blaster/cluster or additional supported options for either kind of clients, for as long as they are compatible with existing prod configuration. We are not going to spend time ourselves trying to refine the redis blaster experience I think, rather we're looking into dropping redis blaster completely.

[Agalin]

It’s strictly about Redis Cluster configuration. Currently it’s simply not possible to some clusters because only a few options can be configured.

[untitaker]

Yeah makes sense. Either way we're happy to merge PRs that go in the direction of making this experience better, but we're not going to actively work on it right now.

On Wed, Nov 16, 2022 at 4:24 PM Agalin @.***> wrote:

It’s strictly about Redis Cluster configuration. Currently it’s simply not possible to some clusters because only a few options can be configured.

— Reply to this email directly, view it on GitHub https://github.com/getsentry/sentry/issues/40762#issuecomment-1317193824, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGMPRKDLBX5O2IQBL23MXLWIT4BJANCNFSM6AAAAAARTEKFIU . You are receiving this because you commented.Message ID: @.***>