Refactor and enhance Redis::Distributed #817

supercaracal · 2018-12-23T11:47:19Z

Hi,

Redis::Distributed give us partitioning feature by client side consistent hashing.
There are some gems which depend on the above feature. e.g. redis-store, ActiveSupport, resque

I'd say that we might be felt pain or exhausting by simultaneous maintaining both Redis and Redis::Distributed as public interfaces. So I refactored the Redis::Distributed along with several additional Redis API supports (e.g. Streams) and several compatibility-breaking minor changes.

Before the changes

Redis::Distributed
  |
  +--- Redis
  |
  +--- HashRing

require 'redis/distributed'

Redis::Distributed.new(nodes, tag: tag, ring: ring, timeout: 10)

After the changes

Redis
  |
  +--- Client
  |
  +--- Cluster
  |      |
  |      +--- Client
  |
  +--- Distributed::Partitioner <- Added
         |
         +--- Client
         |
         +--- Distributed::HashRing <- Namespace changed

Redis.new(distributed: { nodes: nodes, tag: tag, ring: ring }, timeout: 10)

But we leave Redis::Distributed's old interface for backward compatibility.

About several commands that can operate on multiple keys

The current implementation doesn't allow multiple keys without tag. After refactoring, the multiple keys are allowed only if they effect single node.

Compatibility-breaking changes

Redis::Distributed#[] has been removed.
Redis::Distributed#[]= has been removed.
Redis::HashRing#ring has been removed.
Redis::HashRing#sorted_keys has been removed.
Redis::HashRing#replicas has been removed.
Redis::Distributed#on_each_node has been removed in protected scope.
Redis::Distributed#node_index_for has been removed in protected scope.
Redis::Distributed#key_tag has been removed in protected scope.
Redis::Distributed#ensure_same_node has been removed in protected scope.
Redis::Distributed#select has been become raising Redis::Distributed::CannotDistribute error.
The instance type of Redis::Distributed#nodes's return value has been changed Array<Redis> into Array<Redis::Client>.
The nested depth of Redis::Distributed#script(:exists)'s return value has been become 1 from 2.
The raising error has been changed NotImplementedError into Redis::Distributed::CannotDistribute when Redis::Distributed#monitor is called.
The raising error has been changed NoMethodError into Redis::CommandError when any Redis::Distributed#not_yet_implemented_commands are called.

ref: #219
ref: rails/rails#31134
ref: redis-store/redis-store#277
ref: #687
ref: redis-store/redis-store#282
ref: #653 (comment)
ref: #716

supercaracal · 2018-12-25T13:09:13Z

lib/redis/distributed/hash_ring.rb

+      attr_reader :nodes
+
+      # Find the closest index in HashRing with value <= the given value
+      def self.binary_search(ary, value)


It seems that this is subtly different from Array#bsearch_index.

supercaracal · 2018-12-25T13:17:31Z

lib/redis/distributed/partitioner.rb

+      end
+
+      def db=(_db)
+        raise CannotDistribute, 'select'


This error handling is on purpose because HashRing is broken by SELECT command. The checksum calculation depends on the client id. It includes the db number if the id option isn't specified.

supercaracal · 2018-12-25T14:56:41Z

test/distributed_blocking_commands_test.rb

    end
  end

  def test_brpop_raises
-    assert_raises(Redis::Distributed::CannotDistribute) do
-      r.brpop(%w[foo bar])
+    target_version('3.2.0') do


About BRPOP command:

v3.0 The number of last key position is expected to -2 but was 1.

v5.0

supercaracal · 2018-12-25T15:20:46Z

I wonder if the client side sharding feature is still used by someone. I think it is alone not high available. So it seems that it should be combined replication with Sentinel or Keepalived and so on in production environment. But now, we're able to use Redis Cluster.

Data store or cache?

There might be demands as a cache store.

supercaracal · 2019-01-06T08:47:04Z

test/distributed_test.rb

+    rd.set('foo', '1')
+    assert_equal '1', rd.get('foo')
+
+    hr = Redis::HashRing.new


This is used by some gems. e.g. redis-store

supercaracal · 2019-01-11T04:54:05Z

test/distributed_commands_on_value_types_test.rb

  end

  def test_migrate
    r.set("foo", "s1")

    assert_raise Redis::Distributed::CannotDistribute do
-      r.migrate("foo", {})
+      r.migrate('foo', host: '127.0.0.1', port: PORT)


Diff:

Redis::Distributed#migrate

Redis#migrate

supercaracal · 2019-01-11T05:04:17Z

test/distributed_commands_on_value_types_test.rb

@@ -6,6 +6,14 @@ class TestDistributedCommandsOnValueTypes < Test::Unit::TestCase
  include Helper::Distributed
  include Lint::ValueTypes

+  def test_move
+    assert_raise(Redis::Distributed::CannotDistribute) { super }


It is raised from SELECT.

supercaracal · 2019-01-11T05:10:10Z

test/distributed_remote_server_control_commands_test.rb

@@ -27,7 +27,7 @@ def test_info

  def test_info_commandstats
    target_version "2.5.7" do
-      r.nodes.each { |n| n.config(:resetstat) }
+      r.nodes.each { |n| n.call(%i[config resetstat]) }


This is a compatibility-breaking change. The return value's instance type is changed Redis into Redis::Client.

supercaracal · 2019-01-11T05:17:12Z

test/distributed_transactions_test.rb

    assert_raise Redis::Distributed::CannotDistribute do
-      r.multi { @foo = 1 }
+      r.multi { :dummy }


Diff:

Redis::Distributed#multi

Redis#multi

byroot · 2019-05-06T11:57:52Z

I realize I should have said that months ago, but I'm afraid +798 −1,238 is way too big of a change for me to efficiently review, as I'm not that familiar with the codebase myself yet, and even less so with Redis::Cluster.

Is there any way this could be split in more reasonable chunks ? e.g ~300 line max changes. That would make my life much much easier.

supercaracal · 2019-05-06T12:38:55Z

Sure. Thank you for your response. I will try to split this PR. 😄

Move to subdirectory

137716c

supercaracal force-pushed the refactor-distributed branch 4 times, most recently from 60cb738 to 827a999 Compare December 24, 2018 15:00

supercaracal commented Dec 25, 2018

View reviewed changes

supercaracal force-pushed the refactor-distributed branch 2 times, most recently from 0805783 to a42ab18 Compare December 25, 2018 14:17

supercaracal commented Dec 25, 2018

View reviewed changes

supercaracal force-pushed the refactor-distributed branch 2 times, most recently from cd2b24f to fc5cac9 Compare January 6, 2019 08:35

supercaracal commented Jan 6, 2019

View reviewed changes

Refactor and enhance Redis::Distributed

0fb5ef1

supercaracal force-pushed the refactor-distributed branch from fc5cac9 to 0fb5ef1 Compare January 11, 2019 04:23

supercaracal commented Jan 11, 2019

View reviewed changes

supercaracal closed this May 6, 2019

supercaracal deleted the refactor-distributed branch May 2, 2024 11:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and enhance Redis::Distributed #817

Refactor and enhance Redis::Distributed #817

supercaracal commented Dec 23, 2018 •

edited

Loading

supercaracal Dec 25, 2018 •

edited

Loading

supercaracal Dec 25, 2018 •

edited

Loading

supercaracal Dec 25, 2018 •

edited

Loading

supercaracal commented Dec 25, 2018 •

edited

Loading

supercaracal Jan 6, 2019

supercaracal Jan 11, 2019

supercaracal Jan 11, 2019

supercaracal Jan 11, 2019

supercaracal Jan 11, 2019

byroot commented May 6, 2019

supercaracal commented May 6, 2019 •

edited

Loading

Refactor and enhance Redis::Distributed #817

Refactor and enhance Redis::Distributed #817

Conversation

supercaracal commented Dec 23, 2018 • edited Loading

Before the changes

After the changes

About several commands that can operate on multiple keys

Compatibility-breaking changes

supercaracal Dec 25, 2018 • edited Loading

Choose a reason for hiding this comment

supercaracal Dec 25, 2018 • edited Loading

Choose a reason for hiding this comment

supercaracal Dec 25, 2018 • edited Loading

Choose a reason for hiding this comment

supercaracal commented Dec 25, 2018 • edited Loading

supercaracal Jan 6, 2019

Choose a reason for hiding this comment

supercaracal Jan 11, 2019

Choose a reason for hiding this comment

supercaracal Jan 11, 2019

Choose a reason for hiding this comment

supercaracal Jan 11, 2019

Choose a reason for hiding this comment

supercaracal Jan 11, 2019

Choose a reason for hiding this comment

byroot commented May 6, 2019

supercaracal commented May 6, 2019 • edited Loading

supercaracal commented Dec 23, 2018 •

edited

Loading

supercaracal Dec 25, 2018 •

edited

Loading

supercaracal Dec 25, 2018 •

edited

Loading

supercaracal Dec 25, 2018 •

edited

Loading

supercaracal commented Dec 25, 2018 •

edited

Loading

supercaracal commented May 6, 2019 •

edited

Loading