From f03ad02f2db4ceff305d83eebc5c52a2c1c715bc Mon Sep 17 00:00:00 2001
From: Michael Wiencek <mwtuea@gmail.com>
Date: Tue, 8 Mar 2022 16:37:03 -0600
Subject: [PATCH] Add DataStore::RedisMulti

If the `DataStore::RedisMulti` module is active, then
`DATASTORE_REDIS_ARGS` may return an array ref of connection details.
(How do you know if it's active? grep for `DataStore::RedisMulti->new`.
We may revert back to plain-old `DataStore::Redis` if multiple instances
aren't needed.)

This module is useful when Redis service needs to be migrated to a new
server. We'll attempt to read from each connection in order (returning
the first non-empty result), and also distribute writes to all
connections. This allows time to copy any keys that don't exist on the
new instance from the old instance.
---
 .../Plugin/Session/Store/MusicBrainz.pm       |   4 +-
 lib/MusicBrainz/DataStore/Redis.pm            |  18 ++-
 lib/MusicBrainz/DataStore/RedisMulti.pm       | 140 ++++++++++++++++++
 lib/MusicBrainz/Server/Context.pm             |   4 +-
 t/lib/t/MusicBrainz/DataStore/RedisMulti.pm   | 122 +++++++++++++++
 t/tests.t                                     |   1 +
 6 files changed, 280 insertions(+), 9 deletions(-)
 create mode 100644 lib/MusicBrainz/DataStore/RedisMulti.pm
 create mode 100644 t/lib/t/MusicBrainz/DataStore/RedisMulti.pm

diff --git a/lib/Catalyst/Plugin/Session/Store/MusicBrainz.pm b/lib/Catalyst/Plugin/Session/Store/MusicBrainz.pm
index 84f6bcc44d1..1343669be32 100644
--- a/lib/Catalyst/Plugin/Session/Store/MusicBrainz.pm
+++ b/lib/Catalyst/Plugin/Session/Store/MusicBrainz.pm
@@ -1,6 +1,6 @@
 package Catalyst::Plugin::Session::Store::MusicBrainz;
 use Moose;
-use MusicBrainz::DataStore::Redis;
+use MusicBrainz::DataStore::RedisMulti;
 use MIME::Base64 qw(encode_base64 decode_base64);
 use Storable qw/nfreeze thaw/;
 
@@ -9,7 +9,7 @@ extends 'Catalyst::Plugin::Session::Store';
 has '_datastore' => (
     is => 'rw',
     does => 'MusicBrainz::DataStore',
-    default => sub { return MusicBrainz::DataStore::Redis->new; }
+    default => sub { return MusicBrainz::DataStore::RedisMulti->new; }
 );
 
 sub get_session_data {
diff --git a/lib/MusicBrainz/DataStore/Redis.pm b/lib/MusicBrainz/DataStore/Redis.pm
index dff078bc0de..17270f8f543 100644
--- a/lib/MusicBrainz/DataStore/Redis.pm
+++ b/lib/MusicBrainz/DataStore/Redis.pm
@@ -10,12 +10,20 @@ extends 'MusicBrainz::Redis';
 
 with 'MusicBrainz::DataStore';
 
-sub BUILDARGS {
-    my ($class, %args) = @_;
+around BUILDARGS => sub {
+    my $orig = shift;
+    my $class = shift;
 
-    return \%args if %args;
-    return DBDefs->DATASTORE_REDIS_ARGS;
-}
+    if (@_) {
+        return $class->$orig(@_);
+    }
+
+    my $args = DBDefs->DATASTORE_REDIS_ARGS;
+    if (ref($args) eq 'ARRAY') {
+        die 'Use DataStore::RedisMulti to support an array in DATASTORE_REDIS_ARGS.';
+    }
+    return $class->$orig($args);
+};
 
 has '_json' => (
     is => 'ro',
diff --git a/lib/MusicBrainz/DataStore/RedisMulti.pm b/lib/MusicBrainz/DataStore/RedisMulti.pm
new file mode 100644
index 00000000000..08d07e08cd4
--- /dev/null
+++ b/lib/MusicBrainz/DataStore/RedisMulti.pm
@@ -0,0 +1,140 @@
+package MusicBrainz::DataStore::RedisMulti;
+
+use Moose;
+use DBDefs;
+use MusicBrainz::DataStore::Redis;
+
+# If the `DataStore::RedisMulti` module is active, then
+# `DATASTORE_REDIS_ARGS` may return an array ref of connection details.
+# (How do you know if it's active? grep for
+# `DataStore::RedisMulti->new`. We may revert back to plain-old
+# `DataStore::Redis` if multiple instances aren't needed.)
+#
+# This module is useful when Redis service needs to be migrated to a
+# new server. We'll attempt to read from each connection in order
+# (returning the first non-empty result), and also distribute writes to
+# all connections. This allows time to copy any keys that don't exist
+# on the new instance from the old instance.
+
+has '_redis_instances' => (
+    is => 'rw',
+    isa => 'ArrayRef[MusicBrainz::DataStore::Redis]',
+);
+
+with 'MusicBrainz::DataStore';
+
+around BUILDARGS => sub {
+    my $orig = shift;
+    my $class = shift;
+
+    if (@_) {
+        return $class->$orig(@_);
+    }
+
+    my $args = DBDefs->DATASTORE_REDIS_ARGS;
+    if (ref($args) eq 'HASH') {
+        $args = [$args];
+    } elsif (ref($args) ne 'ARRAY') {
+        die 'DATASTORE_REDIS_ARGS must return a HASH or ARRAY ref.';
+    }
+
+    $class->$orig({
+        _redis_instances => [map { MusicBrainz::DataStore::Redis->new($_) } @$args],
+    });
+};
+
+sub _is_non_empty_hash_ref { ref($_[0]) eq 'HASH' && %{ $_[0] } }
+
+sub _is_defined { defined $_[0] }
+
+sub _is_truthy { $_[0] }
+
+sub _is_non_empty_list { scalar(@_) }
+
+sub _exec_all { 0 }
+
+sub _exec_method_wantarray {
+    my ($self, $method, $done, @args) = @_;
+
+    my @ret;
+    for my $instance (@{ $self->_redis_instances }) {
+        @ret = $instance->$method(@args);
+        last if $done->(@ret);
+    }
+    return @ret;
+}
+
+sub _exec_method_wantscalar {
+    my ($self, $method, $done, @args) = @_;
+
+    my @ret = $self->_exec_method_wantarray($method, $done, @args);
+    return $ret[0];
+}
+
+sub clear {
+    shift->_exec_method_wantscalar('clear', \&_exec_all, @_);
+}
+
+sub delete_multi {
+    shift->_exec_method_wantscalar('delete_multi', \&_exec_all, @_);
+}
+
+sub delete {
+    shift->_exec_method_wantscalar('delete', \&_exec_all, @_);
+}
+
+sub disconnect {
+    shift->_exec_method_wantscalar('disconnect', \&_exec_all, @_);
+}
+
+sub exists {
+    shift->_exec_method_wantscalar('exists', \&_is_truthy, @_);
+}
+
+sub get_multi {
+    shift->_exec_method_wantscalar('get_multi', \&_is_non_empty_hash_ref, @_);
+}
+
+sub get {
+    shift->_exec_method_wantscalar('get', \&_is_defined, @_);
+}
+
+sub remove {
+    shift->delete(@_);
+}
+
+sub set_add {
+    shift->_exec_method_wantscalar('set_add', \&_exec_all, @_);
+}
+
+sub set_members {
+    shift->_exec_method_wantarray('set_members', \&_is_non_empty_list, @_);
+}
+
+sub set_multi {
+    shift->_exec_method_wantscalar('set_multi', \&_exec_all, @_);
+}
+
+sub set {
+    shift->_exec_method_wantscalar('set', \&_exec_all, @_);
+}
+
+sub expire {
+    shift->_exec_method_wantscalar('expire', \&_exec_all, @_);
+}
+
+sub expire_at {
+    shift->_exec_method_wantscalar('expire_at', \&_exec_all, @_);
+}
+
+=head1 COPYRIGHT AND LICENSE
+
+Copyright (C) 2022 MetaBrainz Foundation
+
+This file is part of MusicBrainz, the open internet music database,
+and is licensed under the GPL version 2, or (at your option) any
+later version: http://www.gnu.org/licenses/gpl-2.0.txt
+
+=cut
+
+1;
diff --git a/lib/MusicBrainz/Server/Context.pm b/lib/MusicBrainz/Server/Context.pm
index 47d6c6d84b0..c408a3b8426 100644
--- a/lib/MusicBrainz/Server/Context.pm
+++ b/lib/MusicBrainz/Server/Context.pm
@@ -2,7 +2,7 @@ package MusicBrainz::Server::Context;
 use Moose;
 
 use DBDefs;
-use MusicBrainz::DataStore::Redis;
+use MusicBrainz::DataStore::RedisMulti;
 use MusicBrainz::Server::Replication qw( :replication_type );
 use MusicBrainz::Server::CacheManager;
 use aliased 'MusicBrainz::Server::DatabaseConnectionFactory';
@@ -83,7 +83,7 @@ has store => (
     is => 'ro',
     does => 'MusicBrainz::DataStore',
     lazy => 1,
-    default => sub { MusicBrainz::DataStore::Redis->new }
+    default => sub { MusicBrainz::DataStore::RedisMulti->new }
 );
 
 # This is not the Catalyst stash, but it's used by
diff --git a/t/lib/t/MusicBrainz/DataStore/RedisMulti.pm b/t/lib/t/MusicBrainz/DataStore/RedisMulti.pm
new file mode 100644
index 00000000000..a526ccde008
--- /dev/null
+++ b/t/lib/t/MusicBrainz/DataStore/RedisMulti.pm
@@ -0,0 +1,122 @@
+package t::MusicBrainz::DataStore::RedisMulti;
+
+use utf8;
+
+use Test::Routine;
+use Test::Moose;
+use Test::More;
+use Test::Deep qw( cmp_bag );
+use MusicBrainz::Server::Test;
+use MusicBrainz::DataStore::Redis;
+use MusicBrainz::DataStore::RedisMulti;
+use DBDefs;
+
+=head2 Test description
+
+This test checks basic tasks for the Redis store, including adding, deleting
+and expiring keys.
+
+=cut
+
+# Initialize tests
+my $args1 = DBDefs->DATASTORE_REDIS_ARGS;
+$args1->{database} = $args1->{test_database};
+
+my $args2 = DBDefs->DATASTORE_REDIS_ARGS;
+$args2->{database} = $args2->{test_database} + 1;
+
+my $redis1 = MusicBrainz::DataStore::Redis->new($args1);
+my $redis2 = MusicBrainz::DataStore::Redis->new($args2);
+
+my $redis_multi = MusicBrainz::DataStore::RedisMulti->new(
+    _redis_instances => [$redis1, $redis2],
+);
+
+# This doesn't test RedisMulti behavior specifically, but is a prerequisite
+# for the rest of these tests to make sense.
+test 'Databases are separate' => sub {
+    $redis1->set('kx1', 'vx1');
+    $redis2->set('kx2', 'vx2');
+
+    is($redis1->get('kx1'), 'vx1', 'Expected string is in database 1');
+    is($redis2->exists('kx1'), 0, 'String from database 1 is not in database 2');
+
+    is($redis2->get('kx2'), 'vx2', 'Expected string is in database 2');
+    is($redis1->exists('kx2'), 0, 'String from database 2 is not in database 1');
+
+    $redis1->clear;
+    $redis2->clear;
+};
+
+test 'Test key setting/retrieving' => sub {
+    is($redis_multi->get('does-not-exist'), undef, 'Non-existent key returns undef');
+
+    $redis_multi->set('string', 'Esperándote');
+    is($redis_multi->get('string'), 'Esperándote', 'Retrieved expected string');
+    is($redis1->get('string'), 'Esperándote', 'Expected string is in database 1');
+    is($redis2->get('string'), 'Esperándote', 'Expected string is in database 2');
+
+    $redis1->set('string-1-only', '1-only');
+    $redis1->set('string-2-only', '2-only');
+    is($redis_multi->get('string-1-only'), '1-only', 'Retrieved string that was only set in database 1');
+    is($redis_multi->get('string-2-only'), '2-only', 'Retrieved string that was only set in database 2');
+
+    is($redis_multi->exists('does-not-exist'), 0, 'exists returns 0 for non-existent key');
+    is($redis1->exists('does-not-exist'), 0, 'exists returns 0 for non-existent key in database 1');
+    is($redis2->exists('does-not-exist'), 0, 'exists returns 0 for non-existent key in database 2');
+
+    is($redis_multi->exists('string'), 1, 'exists returns 1 for existing key');
+    is($redis1->exists('string'), 1, 'exists returns 1 for existing key in database 1');
+    is($redis2->exists('string'), 1, 'exists returns 1 for existing key in database 2');
+
+    $redis_multi->delete('string');
+    is($redis_multi->exists('string'), 0, 'exists returns 0 for deleted key');
+    is($redis1->exists('string'), 0, 'exists returns 0 for deleted key in database 1');
+    is($redis2->exists('string'), 0, 'exists returns 0 for deleted key in database 2');
+
+    $redis_multi->set_multi(['k1', 'v1'], ['k2', 'v2']);
+    is_deeply(
+        $redis_multi->get_multi('k1', 'k2'),
+        { k1 => 'v1', k2 => 'v2' },
+        'Retrieved expected multiple values',
+    );
+    is_deeply(
+        $redis1->get_multi('k1', 'k2'),
+        { k1 => 'v1', k2 => 'v2' },
+        'Retrieved expected multiple values from database 1',
+    );
+    is_deeply(
+        $redis2->get_multi('k1', 'k2'),
+        { k1 => 'v1', k2 => 'v2' },
+        'Retrieved expected multiple values from database 2',
+    );
+
+    $redis_multi->delete_multi('k1', 'k2');
+    is($redis_multi->exists('k1'), 0, 'exists returns 0 for first deleted key');
+    is($redis_multi->exists('k2'), 0, 'exists returns 0 for second deleted key');
+    is($redis1->exists('k1'), 0, 'exists returns 0 for first deleted key in database 1');
+    is($redis1->exists('k2'), 0, 'exists returns 0 for second deleted key in database 1');
+    is($redis2->exists('k1'), 0, 'exists returns 0 for first deleted key in database 2');
+    is($redis2->exists('k2'), 0, 'exists returns 0 for second deleted key in database 2');
+
+    $redis_multi->set_add('setk', qw( v1 v2 v3 ));
+    my @set_values = $redis_multi->set_members('setk');
+    cmp_bag(\@set_values, [qw( v1 v2 v3 )], 'Retrieved expected set members');
+    @set_values = $redis1->set_members('setk');
+    cmp_bag(\@set_values, [qw( v1 v2 v3 )], 'Retrieved expected set members from database 1');
+    @set_values = $redis2->set_members('setk');
+    cmp_bag(\@set_values, [qw( v1 v2 v3 )], 'Retrieved expected set members from database 2');
+
+    $redis_multi->clear;
+};
+
+test 'Test setting key expiration' => sub {
+    $redis_multi->set('int', 23);
+    $redis_multi->expire_at('int', time() + 2);
+    ok($redis_multi->exists('int'), 'int still exists');
+    sleep(2);
+    ok(!$redis_multi->exists('int'), 'int no longer exists');
+    $redis_multi->clear;
+};
+
+1;
diff --git a/t/tests.t b/t/tests.t
index fefac0fd33a..63c95f73fa3 100644
--- a/t/tests.t
+++ b/t/tests.t
@@ -11,6 +11,7 @@ use MusicBrainz::Server::Test qw( commandline_override );
 my @classes = (
     't::Sql',
     't::MusicBrainz::DataStore::Redis',
+    't::MusicBrainz::DataStore::RedisMulti',
     't::MusicBrainz::Script::RebuildCoverArt',
     't::MusicBrainz::Script::RemoveEmptyURLs',
     map {