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

Connect to ENV["REDIS_URL"] by default #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

foca
Copy link

@foca foca commented Jan 14, 2019

This makes it so that if you have a REDIS_URL environment variable set, redic will connect to that, and if that's not set it will connect to the default redis://127.0.0.1:6379 address.

cc @cyx @soveran

This makes it so that if you have a `REDIS_URL` environment variable set,
redic will connect to that, and if that's not set it will connect to the default
`redis://127.0.0.1:6379` address.
@soveran
Copy link
Contributor

soveran commented Jan 17, 2019

That used to be the default, but we removed it in a668e4b because we wanted to make the connection explicit when the client is instantiated, with "redis://127.0.0.1:6379" as the only default value.

@foca
Copy link
Author

foca commented Jan 19, 2019

Hrm. I thought that was the default! I set up Redic for the first time in a couple years and it broke a deploy because I assumed it would do this.

Locally and in CI everything worked because REDIS_URL just pointed to redis://127.0.0.1:6379, but when it got to production it broke.

I wonder if the goal is to be explicit whether Redic should be having any default at all. Let it fail loudly with an ArgumentError if you do Redic.new with no value. Then you're free to make that value configurable however you want.

WDYT? Feel free to close the issue whatever you decide.

@JokerCatz
Copy link

JokerCatz commented Jan 19, 2019

@foca I overwrite Redic initialize method & add MULTI support in my application (only for multi write / update)

require 'redic'
require 'thread'
class Redic
  def initialize(options = {})
    options = {:host => "0.0.0.0", :port => 6379, :db => '' , :timeout => 10_000_000}.merge(options)
    @url = options[:url] ? "#{options[:url]}/#{options[:db]}" : "redis://#{options[:host]}:#{options[:port]}/#{options[:db]}"
    @client = Redic::Client.new(@url, options[:timeout])
    @queue = []
    @mutex = Mutex.new
  end
  def queue_source
    @queue
  end
  def queue(*args)
    @queue << args
  end
  def exec!
    @client.connect do
      @client.write(['MULTI'])
      @queue.each do |args|
        @client.write(args)
      end
      @client.write(['EXEC'])
      @client.read
      @queue.each do
        @client.read #ans = "QUEUE"
      end
      @client.read #return ans
    end
  ensure
    @queue.clear
  end
end

Im try to make it like Redis initialize : ) , but it only support for old version of redic ...

@soveran
Copy link
Contributor

soveran commented Jan 31, 2019

Hrm. I thought that was the default! I set up Redic for the first time in a couple years and it broke a deploy because I assumed it would do this.

Locally and in CI everything worked because REDIS_URL just pointed to redis://127.0.0.1:6379, but when it got to production it broke.

That's a big gotcha. In the docs it's stated that redis://127.0.0.1:6379 is the default, and there's one example of reading the REDIS_URL environment variable and passing it to Redic.new.

I wonder if the goal is to be explicit whether Redic should be having any default at all. Let it fail loudly with an ArgumentError if you do Redic.new with no value. Then you're free to make that value configurable however you want.

I think the convenience of the default is the same as the convenience of running redis-server and getting it running on port 6379, but what I think can be improved is how this default is described in the docs. Right now it's in a comment in one of the examples and it should be even more explicit and clarify the fact that it doesn't read environment variables.

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