Use block parameter to pipeline in Redis#multi #68

capripot · 2022-02-18T16:07:33Z

In preparation of Redis 5, as info message alerts.

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

(called from /app/vendor/bundle/ruby/2.7.0/gems/kredis-1.0.1/lib/kredis/types/proxy.rb:13:in `multi'}>

Using block parameter is compatible down to redis gem version 3.0, current dependency to redis is defined as ~> 4.2.

In preparation of Redis 5, as info message alerts. > 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 > > (called from /app/vendor/bundle/ruby/2.7.0/gems/kredis-1.0.1/lib/kredis/types/proxy.rb:13:in `multi'}>

dhh · 2022-02-19T07:47:28Z

Looks like tests are failing?

capripot · 2022-02-21T08:17:42Z

Ha true! My bad. I just fixed the calls adding the key to them, now bin/test is all green! ✅

capripot · 2022-02-21T08:21:51Z

lib/kredis/types/unique_list.rb

-    multi do
-      remove elements
+    multi do |pipeline|
+      types_to_strings(elements, typed).each { |element| pipeline.lrem key, 0, element }


It repeats List#remove's code, but making remove accept a pipeline optional parameter looked equally unideal.

Man, this is a pretty nasty API regression. Need to take a look and see what else we can come up with.

lib/kredis/types/counter.rb

dhh · 2022-02-21T09:49:01Z

lib/kredis/types/set.rb

-      add members
+    multi do |pipeline|
+      pipeline.del key
+      pipeline.sadd key, types_to_strings(members, typed) if members.flatten.any?


Need to extract types_to_strings(members, typed) if members.flatten.any? into a method. It's repeated three times.

dhh · 2022-02-21T09:50:53Z

lib/kredis/types/unique_list.rb

-    multi do
-      remove elements
+    multi do |pipeline|
+      types_to_strings(elements, typed).each { |element| pipeline.lrem key, 0, element }


Man, this is a pretty nasty API regression. Need to take a look and see what else we can come up with.

capripot · 2022-02-21T17:55:52Z

Here is what I came up with:

using a new proxy within Kredis#multi as Kredis::Types::Proxy.new(pipeline, key), it has the advantage of having same interface as with redis
for "shared" methods, I used an optional keyword argument

We could also have an optional keyword argument on all proxied methods, that would create less objects, but could be a little inelegant as we would need to pass pipeline: pipeline everywhere, beyond "shared" methods.

class Kredis::Types::Proxy
  def method_missing(method, *args, **kwargs)
    Kredis.instrument :proxy, **log_message(method, *args, **kwargs) do
      failsafe do
        (kwargs.delete(:pipeline) || redis).public_send method, key, *args, **kwargs
      end
    end
  end

Let me know if you have any other ideas!

capripot · 2022-02-21T17:56:44Z

lib/kredis/types/set.rb

  end
  alias << add

-  def remove(*members)
-    srem types_to_strings(members, typed) if members.flatten.any?
+  def remove(*members, pipeline: nil)


For consistency

lewispb · 2022-03-01T22:00:50Z

Played with this and came up with something a little simpler: #78

dhh · 2022-04-30T10:50:30Z

Going to merge this for now. @lewispb we can take a swing at the ivar version if we determine that's a thread safe approach afterwards.

* main: Bump version for 1.2.0 Use pipeline for migration too Ensure to pass the pipeline to super Use block parameter to pipeline in Redis#multi (rails#68) Note tested / supported versions of Redis in the Readme (rails#79) Return counter value after incrementing/decrementing Enum Bang setter (rails#82) Add reset to Cycle after_change callbacks Add example in Readme.md Support configuring custom keys via method invocation Use bundle add instead (rails#81)

This reverts commit 88b8c29.

* main: Bump version for 1.2.0 Use pipeline for migration too Ensure to pass the pipeline to super Use block parameter to pipeline in Redis#multi (rails#68) Note tested / supported versions of Redis in the Readme (rails#79) Return counter value after incrementing/decrementing Enum Bang setter (rails#82) Add reset to Cycle after_change callbacks Add example in Readme.md Support configuring custom keys via method invocation Use bundle add instead (rails#81)

* main: Coalesce "current pipeline or redis" into the redis method itself Pefer a thread_mattr_accessor over a thread local variable Delete list of keys in batch (rails#90) Use a thread-local variable for pipeline Revert "Use block parameter to pipeline in Redis#multi (rails#68)" Resolve pipelining deprecation warnings

* main: (21 commits) Bump version for 1.4.0 Update nokogiri for compatibility Revert "Improved version of UniqueList: OrderedSet (rails#76)" (rails#111) Add `last` to lists (rails#97) Improved version of UniqueList: OrderedSet (rails#76) Return Time objects instead of deprecated DateTime (rails#106) Fix possible deserialization of untrusted data Typecast return of Set#take (rails#105) Declare Active Model dependency (rails#107) Address LogSubscriber deprecation (rails#98) Account for time zones in DateTime serializations (rails#102) Add sample to set (rails#100) Bump version for 1.3.0 Allow Redis 5.x Add ltrim to lists Coalesce "current pipeline or redis" into the redis method itself Pefer a thread_mattr_accessor over a thread local variable Delete list of keys in batch (rails#90) Use a thread-local variable for pipeline Revert "Use block parameter to pipeline in Redis#multi (rails#68)" ...

Pipelined methods need key to be passed along

b2ee8e4

capripot force-pushed the use-block-var-to-pipeline-in-multi branch from e176a1e to b2ee8e4 Compare February 21, 2022 08:17

capripot commented Feb 21, 2022

View reviewed changes

dhh reviewed Feb 21, 2022

View reviewed changes

capripot force-pushed the use-block-var-to-pipeline-in-multi branch 2 times, most recently from 87cfd29 to e164798 Compare February 21, 2022 17:44

Proxying multi

8a40c02

capripot force-pushed the use-block-var-to-pipeline-in-multi branch from e164798 to 8a40c02 Compare February 21, 2022 17:46

capripot commented Feb 21, 2022

View reviewed changes

jeremy mentioned this pull request Apr 25, 2022

Avoid having to pass a Redis pipeline argument around #78

Merged

Merge branch 'main' into use-block-var-to-pipeline-in-multi

ee844f5

dhh merged commit 88b8c29 into rails:main Apr 30, 2022

lewispb added a commit to lewispb/kredis that referenced this pull request May 2, 2022

Revert "Use block parameter to pipeline in Redis#multi (rails#68)"

6c2a8a2

This reverts commit 88b8c29.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use block parameter to pipeline in Redis#multi #68

Use block parameter to pipeline in Redis#multi #68

capripot commented Feb 18, 2022

dhh commented Feb 19, 2022

capripot commented Feb 21, 2022

capripot Feb 21, 2022 •

edited

dhh Feb 21, 2022

dhh Feb 21, 2022

dhh Feb 21, 2022

capripot commented Feb 21, 2022 •

edited

capripot Feb 21, 2022

lewispb commented Mar 1, 2022

dhh commented Apr 30, 2022

Use block parameter to pipeline in Redis#multi #68

Use block parameter to pipeline in Redis#multi #68

Conversation

capripot commented Feb 18, 2022

dhh commented Feb 19, 2022

capripot commented Feb 21, 2022

capripot Feb 21, 2022 • edited

Choose a reason for hiding this comment

dhh Feb 21, 2022

Choose a reason for hiding this comment

dhh Feb 21, 2022

Choose a reason for hiding this comment

dhh Feb 21, 2022

Choose a reason for hiding this comment

capripot commented Feb 21, 2022 • edited

capripot Feb 21, 2022

Choose a reason for hiding this comment

lewispb commented Mar 1, 2022

dhh commented Apr 30, 2022

capripot Feb 21, 2022 •

edited

capripot commented Feb 21, 2022 •

edited