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 Cluster URL #887

Open
sj26 opened this issue Jan 30, 2020 · 10 comments
Open

Redis Cluster URL #887

sj26 opened this issue Jan 30, 2020 · 10 comments

Comments

@sj26
Copy link

sj26 commented Jan 30, 2020

It's a common pattern to configure an application with something like:

redis = Redis.new(ENV["REDIS_URL"])

When migration from a single Redis instance to a redis cluster, this become a little awkward, because the ruby redis library must be explicitly put into cluster mode:

redis = if ENV["REDIS_CLUSTER"]
          Redis.new(cluster: [ENV["REDIS_URL"]])
        else
          Redis.new(ENV["REDIS_URL"])
        end

Especially if you have multiple redis connections

redis = if ENV["REDIS_CLUSTER"]
          Redis.new(cluster: [ENV["REDIS_URL"]])
        else
          Redis.new(ENV["REDIS_URL"])
        end

sidekiq_redis = if ENV["SIDEKIQ_REDIS_CLUSTER"]
                  Redis.new(cluster: [ENV["SIDEKIQ_REDIS_URL"]])
                else
                  Redis.new(ENV["SIDEKIQ_REDIS_URL"])
                end

and defaults:

redis = if ENV["REDIS_CLUSTER"]
          Redis.new(cluster: [ENV["REDIS_URL"]])
        else
          Redis.new(ENV["REDIS_URL"])
        end

sidekiq_redis = if ENV["SIDEKIQ_REDIS_URL"]
                  if ENV["SIDEKIQ_REDIS_CLUSTER"]
                    Redis.new(cluster: [ENV["SIDEKIQ_REDIS_URL"]])
                  else
                    Redis.new(ENV["SIDEKIQ_REDIS_URL"])
                  end
                else
                  redis
                end

It'd be super nice if we could just change the URL to indicate that it is a cluster URL. Maybe a query parameter in the style of DATABASE_URL=postgres://localhost/name?encrypt=true parameters like:

Redis.new("redis://localhost?cluster=true")

or indicate that the connection mode is fundamentally different by using a different scheme:

Redis.new("redis+cluster://localhost")

which pares that later example back to:

redis = Redis.new(ENV["REDIS_URL"])
sidekiq_redis = Redis.new(ENV.fetch("SIDEKIQ_REDIS_URL", ENV["REDIS_URL"]))

Would there be interest in a PR to implement either the query param or URL scheme option? I'm leaning toward the query parameter because most redis libraries don't seem to need to differentiate, so there isn't a precedent to change the scheme, and it's technically the same protocol.

@supercaracal
Copy link
Contributor

supercaracal commented Feb 3, 2020

Hello,

I would say that we should keep consistency as a client's API if we implement the feature.

  • Standalone mode
  • Sentinel mode
  • Cluster mode

I think we can ignore Redis::Distributed as a client side consistent hashing feature in this case.

@simi
Copy link

simi commented Oct 2, 2023

@byroot would be contribution of support for REDIS_SENTINEL_URLS welcomed?

@casperisfine
Copy link

casperisfine commented Oct 3, 2023

would be contribution of support for REDIS_SENTINEL_URLS welcomed?

Yes and no :)

I don't really want the gem to read magic environment variables, IMO reading $REDIS_URL in Redis#initialize is a mistake / historical cruft, it should be the application that passes the url.

However, I'm open to changes that would make instantiating a client with $REDIS_SENTINEL_URLS a one liner, e.g. Redis.new(sentinel_urls: ENV["REDIS_SENTINEL_URLS"]) or something like that.

@simi
Copy link

simi commented Oct 3, 2023

@casperisfine alternatively what about to support some kind of REDIS_URL with query parameters to setup various options like sentinels, role, ...?

@casperisfine
Copy link

Yeah, worth exploring. I suppose a combinaison of scheme and query parameters might work.

Would be worth having a quick look if other clients have something similar, or if some hosting services already have a convention for this stuff.

@simi
Copy link

simi commented Oct 3, 2023

Would be worth having a quick look if other clients have something similar, or if some hosting services already have a convention for this stuff.

I quickly check other libraries and there is no support for sentinel setup with ENV vars. :(

@casperisfine
Copy link

there is no support for sentinel setup with ENV vars. :(

kk, thanks for looking.

@simi
Copy link

simi commented Oct 3, 2023

I'll take another look and try to bring back some ideas.

@arusa
Copy link

arusa commented Jan 24, 2024

There's even a really old gem that adds the functionality of setting a "redis+sentinel://" URL to the redis-rb gem. As there were no updates to this gem since 6 years I hoped that this feature was already included in redis-rb.

I'm currently planning to use a redis+sentinel replication on Kubernetes as a target for sidekiq and it would be very helpful if I could use the RAILS_ENV variable to tell the application if it should connect to a single redis instance in dev or a sentinel-setup in production.

@simi
Copy link

simi commented Jan 24, 2024

@arusa for now we have following "naive" approach (it is not bulletproof) in app.

  class Config
    def self.redis
      {
        url: ENV['REDIS_URL'],
        name: ENV['REDIS_SENTINEL_NAME'],
        sentinels: ENV['REDIS_SENTINEL_URLS']&.split(',')&.map { |url| {url: url} },
        driver: :hiredis,
        timeout: 60,
      }.compact
    end
  end

# ...

redis = Redis.new(Config.redis)

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

No branches or pull requests

5 participants