resque / redis-namespace

This gem adds a Redis::Namespace class which can be used to namespace Redis keys.
http://redis.io
MIT License
695 stars 192 forks source link

Pipelining commands on a Redis instance is deprecated and will be removed in Redis 5.0.0 #234

Closed djberg96 closed 5 months ago

djberg96 commented 5 months ago

I'm seeing the following warning message from redis-namespace:

Pipelining commands on a Redis instance is deprecated and will be removed in Redis 5.0.0.

redis.multi do
  redis.get("key")
end

should be replaced by

redis.multi do |pipeline|
  pipeline.get("key")
end

I can replicate it like so:

class SidekiqMiddleware::Stuff
  def call(_worker, job, _queue)
    send_to_morgue(job)
  end

  def send_to_morgue(msg)
    Sidekiq.logger.info { "Adding dead #{msg['class']} job #{msg['jid']}" }
    payload = Sidekiq.dump_json(msg)
    now = Time.now.to_f
    Sidekiq.redis do |conn|
      conn.multi do
        conn.zadd('dead', now, payload)
        conn.zremrangebyscore('dead', '-inf', now - Sidekiq::DeadSet.timeout)
        conn.zremrangebyrank('dead', 0, -Sidekiq::DeadSet.max_jobs)
      end
    end
  end
end

# Just trying to emulate 
job = {'retry_count' => 4, 'retry' => true}
middleware = SidekiqMiddleware::Stuff.new
middleware.call(worker, job, queue){}

This method seems to be the code in question from redis-namespace that triggers it:

def multi(&block)
  if block_given?
    namespaced_block(:multi, &block)
  else
    call_with_namespace(:multi)
  end
end

Can this be fixed? Or should I just silence it?

PatrickTulskie commented 5 months ago

Sorry I'm not super familiar with Sidekiq, but what happens if you change it to:

    Sidekiq.redis.multi do |conn|
      conn.zadd('dead', now, payload)
      conn.zremrangebyscore('dead', '-inf', now - Sidekiq::DeadSet.timeout)
      conn.zremrangebyrank('dead', 0, -Sidekiq::DeadSet.max_jobs)
    end
djberg96 commented 5 months ago

@PatrickTulskie Didn't like it, apparently the .redis method requires a block, so that raises an error.

djberg96 commented 5 months ago

@PatrickTulskie However, this does appear to fix it:

Sidekiq.redis do |conn|
  conn.multi do |pipeline|
    pipeline.zadd('dead', now, payload)
    pipeline.zremrangebyscore('dead', '-inf', now - Sidekiq::DeadSet.timeout)
    pipeline.zremrangebyrank('dead', 0, -Sidekiq::DeadSet.max_jobs)
  end
end
djberg96 commented 5 months ago

Gonna go ahead and close this since I have a fix. Sorry for the noise, and thanks for the assist @PatrickTulskie.

PatrickTulskie commented 5 months ago

Ahh gotcha. That was going to be my next suggestion. Glad you got it fixed @djberg96

djberg96 commented 5 months ago

Well, upon further review, making that change caused some obscure failures in our middleware stack, but the error message doesn't make it clear what the issue was. FWIW, that send_to_morgue code was largely borrowed from their send_to_morgue method which still doesn't use the pipelining.

I guess we'll have to swallow these with Redis.silence_deprecations.