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


-  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

Uh oh!

Use block parameter to pipeline in Redis#multi #68

Use block parameter to pipeline in Redis#multi #68

Uh oh!

capripot commented Feb 18, 2022

Uh oh!

dhh commented Feb 19, 2022

Uh oh!

capripot commented Feb 21, 2022

Uh oh!

capripot Feb 21, 2022 •

edited

Loading

Uh oh!

dhh Feb 21, 2022

Uh oh!

Uh oh!

dhh Feb 21, 2022

Uh oh!

dhh Feb 21, 2022

Uh oh!

capripot commented Feb 21, 2022 •

edited

Loading

Uh oh!

capripot Feb 21, 2022

Uh oh!

lewispb commented Mar 1, 2022

Uh oh!

dhh commented Apr 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Use block parameter to pipeline in Redis#multi #68

Use block parameter to pipeline in Redis#multi #68

Uh oh!

Conversation

capripot commented Feb 18, 2022

Uh oh!

dhh commented Feb 19, 2022

Uh oh!

capripot commented Feb 21, 2022

Uh oh!

capripot Feb 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhh Feb 21, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dhh Feb 21, 2022

Choose a reason for hiding this comment

Uh oh!

dhh Feb 21, 2022

Choose a reason for hiding this comment

Uh oh!

capripot commented Feb 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

capripot Feb 21, 2022

Choose a reason for hiding this comment

Uh oh!

lewispb commented Mar 1, 2022

Uh oh!

dhh commented Apr 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

capripot Feb 21, 2022 •

edited

Loading

capripot commented Feb 21, 2022 •

edited

Loading