amakawa / redic

Lightweight Redis Client
MIT License
120 stars 14 forks source link

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

Open foca opened 5 years ago

foca commented 5 years ago

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

soveran commented 5 years ago

That used to be the default, but we removed it in a668e4b62134e42539bfb15a7c714aeacf5a745b 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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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.