From 25ae12a999ad587b6c961ae741e7886c2e9bff1e Mon Sep 17 00:00:00 2001 From: VeljkoMaksimovic Date: Thu, 4 Aug 2022 12:12:01 +0200 Subject: [PATCH 1/5] Inconsistent handling of connection issues with redis Tests are failing because GenServer processes are terminating because of unhandeled errors. When put/get/fetch operations are executed on redis instances that are not available, no errors are raised. We return usualy nil values, as if the key does not exist, and log the error. Looking at the tests, that is expected from exists? function as well, but thats not the case. When we execute exists? function upon redis instance that cant be reached, error is raised, and entire GenServer is terminated, although in tests, we expect false value, as if the key does not exist. This commit implements the logic that is expected in tests. If redis cant be reached, error is logged, and false value is returned. --- lib/cacheman.ex | 7 ++++++- lib/cacheman/backend/redis.ex | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/cacheman.ex b/lib/cacheman.ex index 1f4848e..568a509 100644 --- a/lib/cacheman.ex +++ b/lib/cacheman.ex @@ -233,7 +233,12 @@ defmodule Cacheman do fully_qualified_key_name(opts, key) ]) - {:reply, response, opts} + if is_boolean(response) do + {:reply, response, opts} + else + Logger.error("Cacheman - #{inspect(response)}") + {:reply, false, opts} + end end def handle_call({:put, key, value, put_opts}, _from, opts) do diff --git a/lib/cacheman/backend/redis.ex b/lib/cacheman/backend/redis.ex index 591d1ff..6406680 100644 --- a/lib/cacheman/backend/redis.ex +++ b/lib/cacheman/backend/redis.ex @@ -25,7 +25,7 @@ defmodule Cacheman.Backend.Redis do case Redix.command(c, ["EXISTS", key]) do {:ok, 1} -> true {:ok, 0} -> false - {:error, reason} -> raise reason + {:error, reason} -> reason end end) end From 0418456d1bb7f5eac059b5294eb0046230340969 Mon Sep 17 00:00:00 2001 From: VeljkoMaksimovic Date: Thu, 4 Aug 2022 14:14:09 +0200 Subject: [PATCH 2/5] Add put_batch for batch inserts --- lib/cacheman.ex | 42 +++++++++++++++++++++++++++ lib/cacheman/backend/redis.ex | 11 +++++++ test/cacheman_test.exs | 54 ++++++++++++++++++++++++++++++++--- 3 files changed, 103 insertions(+), 4 deletions(-) diff --git a/lib/cacheman.ex b/lib/cacheman.ex index 568a509..1c1b2fc 100644 --- a/lib/cacheman.ex +++ b/lib/cacheman.ex @@ -144,6 +144,26 @@ defmodule Cacheman do end end + @type key_value_pair :: {String.t(), String.t()} + @doc """ + Puts a list of key-value pairs into the cache as a part of single request to the redis server + + {:ok, no_of_successful_inserts} = Cacheman.put_batch(:app, [{"key1", "value1"}, {"key2", "value2"}]) + + The response can be one of the following: + + - {:ok, no_of_successful_inserts} - if no errors were raised by Redix + - {:error, redix_error} - if there was an error while communicating with cache backends + + Same as with Put function, TTL can optionally provided + """ + @spec put_batch(String.t(), list(key_value_pair), list()) :: + {:ok, integer} + | {:error, Redix.Protocol.ParseError | Redix.Error | Redix.ConnectionError} + def put_batch(name, key_value_pairs, put_opts \\ @default_put_options) do + GenServer.call(full_process_name(name), {:put_batch, key_value_pairs, put_opts}) + end + @doc """ Fetch is the main entrypoint for caching. The algorithm works like this: @@ -253,6 +273,28 @@ defmodule Cacheman do {:reply, response, opts} end + def handle_call({:put_batch, key_value_pairs, put_opts}, _from, opts) do + response = + apply(opts.backend_module, :put_batch, [ + opts.backend_pid, + Enum.map(key_value_pairs, fn key_value_pair -> + { + fully_qualified_key_name(opts, elem(key_value_pair, 0)), + elem(key_value_pair, 1) + } + end), + put_opts + ]) + + case response do + {:ok, response_values} -> + {:reply, {:ok, response_values |> Enum.count(&(&1 == "OK"))}, opts} + + _ -> + {:reply, response, opts} + end + end + def handle_call({:delete, keys}, _from, opts) do response = apply(opts.backend_module, :delete, [ diff --git a/lib/cacheman/backend/redis.ex b/lib/cacheman/backend/redis.ex index 6406680..4c77692 100644 --- a/lib/cacheman/backend/redis.ex +++ b/lib/cacheman/backend/redis.ex @@ -39,6 +39,17 @@ defmodule Cacheman.Backend.Redis do end) end + def put_batch(conn, key_value_pairs, ttl) when is_list(key_value_pairs) do + list_of_commands = + Enum.map(key_value_pairs, fn pair -> + ["SET", elem(pair, 0), elem(pair, 1)] ++ ttl_command(ttl) + end) + + :poolboy.transaction(conn, fn c -> + Redix.pipeline(c, list_of_commands) + end) + end + def delete(conn, keys) do :poolboy.transaction(conn, fn c -> Redix.command(c, ["DEL"] ++ keys) diff --git a/test/cacheman_test.exs b/test/cacheman_test.exs index b3c4e9d..d6a6ab9 100644 --- a/test/cacheman_test.exs +++ b/test/cacheman_test.exs @@ -39,8 +39,54 @@ defmodule CachemanTest do assert value == content end + test "put_batch" do + key1 = "test-#{:rand.uniform(10_000)}" + key2 = "test-#{:rand.uniform(10_000)}" + + assert {:ok, nil} = Cacheman.get(:good, key1) + assert {:ok, nil} = Cacheman.get(:good, key2) + + assert {:ok, 2} = Cacheman.put_batch(:good, [{key1, "value1"}, {key2, "value2"}]) + + assert {:ok, "value1"} = Cacheman.get(:good, key1) + assert {:ok, "value2"} = Cacheman.get(:good, key2) + end + + test "put_batch with TTL" do + key1 = "test-#{:rand.uniform(10_000)}" + key2 = "test-#{:rand.uniform(10_000)}" + + assert {:ok, nil} = Cacheman.get(:good, key1) + assert {:ok, nil} = Cacheman.get(:good, key2) + + ttl = :timer.seconds(1) + + assert {:ok, 2} = Cacheman.put_batch(:good, [{key1, "value1"}, {key2, "value2"}], ttl: ttl) + + assert {:ok, "value1"} = Cacheman.get(:good, key1) + assert {:ok, "value2"} = Cacheman.get(:good, key2) + + :timer.sleep(ttl) + + assert {:ok, nil} = Cacheman.get(:good, key1) + assert {:ok, nil} = Cacheman.get(:good, key2) + end + + test "put_batch cant reach redis srever" do + key1 = "test-#{:rand.uniform(10_000)}" + key2 = "test-#{:rand.uniform(10_000)}" + + assert {:ok, nil} = Cacheman.get(:good, key1) + assert {:ok, nil} = Cacheman.get(:good, key2) + + assert {:error, _} = Cacheman.put_batch(:broken, [{key1, "value1"}, {key2, "value2"}]) + + assert {:ok, nil} = Cacheman.get(:good, key1) + assert {:ok, nil} = Cacheman.get(:good, key2) + end + test "fetch and store" do - key = "test-#{System.unique_integer([:positive])}" + key = "test-#{:rand.uniform(10_000)}" # at the start, there is no value assert {:ok, nil} = Cacheman.get(:good, key) @@ -56,7 +102,7 @@ defmodule CachemanTest do end test "TTL for keys" do - key = "test-#{System.unique_integer([:positive])}" + key = "test-#{:rand.uniform(10_000)}" ttl = :timer.seconds(1) assert {:ok, "hello"} = Cacheman.put(:good, key, "hello", ttl: ttl) @@ -117,7 +163,7 @@ defmodule CachemanTest do end test "fetch and store" do - key = "test-#{System.unique_integer([:positive])}" + key = "test-#{:rand.uniform(10_000)}" assert {:ok, nil} = Cacheman.get(:broken, key) assert {:ok, "hello"} = Cacheman.fetch(:broken, key, fn -> {:ok, "hello"} end) @@ -128,7 +174,7 @@ defmodule CachemanTest do end test "TTL for keys" do - key = "test-#{System.unique_integer([:positive])}" + key = "test-#{:rand.uniform(10_000)}" ttl = :timer.seconds(1) assert {:error, _} = Cacheman.put(:broken, key, "hello", ttl: ttl) From 0e332540bd63402350887736643c70df3ff1b37c Mon Sep 17 00:00:00 2001 From: VeljkoMaksimovic Date: Thu, 4 Aug 2022 15:03:06 +0200 Subject: [PATCH 3/5] Add get_batch operation --- lib/cacheman.ex | 38 ++++++++++++++++++++++++++++++++--- lib/cacheman/backend/redis.ex | 11 ++++++++++ test/cacheman_test.exs | 30 ++++++++++++++++++++------- 3 files changed, 69 insertions(+), 10 deletions(-) diff --git a/lib/cacheman.ex b/lib/cacheman.ex index 1c1b2fc..f14e64e 100644 --- a/lib/cacheman.ex +++ b/lib/cacheman.ex @@ -81,6 +81,8 @@ defmodule Cacheman do use GenServer require Logger + @default_put_options [ttl: :infinity] + # # Cacheman API # @@ -109,14 +111,25 @@ defmodule Cacheman do The response can be one of the following: - {:ok, value} - if the entry is found - - {:ok, nil} - if the entry is not-found - - {:error, description} - if there was an error while communicating with cache backends + - {:ok, nil} - if the entry is not-found, or redis error occurres """ def get(name, key) do GenServer.call(full_process_name(name), {:get, key}) end - @default_put_options [ttl: :infinity] + @doc """ + Gets a list of values from the cache. + + {:ok, ["value1", "value2"]} = Cacheman.get(:app, ["key1", "key2"]) + + The response can be one of the following: + + - {:ok, list_of_values} - if the entry is found. In place of every key that was not found will be nil value. + All values will be nil if redis cant be reached. + """ + def get_batch(name, keys) when is_list(keys) do + GenServer.call(full_process_name(name), {:get_batch, keys}) + end @doc """ Puts a value into the cache. @@ -246,6 +259,25 @@ defmodule Cacheman do end end + def handle_call({:get_batch, keys}, _from, opts) do + response = + apply(opts.backend_module, :get_batch, [ + opts.backend_pid, + Enum.map(keys, fn key -> + fully_qualified_key_name(opts, key) + end) + ]) + + case response do + {:ok, vals} -> + {:reply, {:ok, vals}, opts} + + e -> + Logger.error("Cacheman - #{inspect(e)}") + {:reply, {:ok, :lists.concat(List.duplicate([nil], length(keys)))}, opts} + end + end + def handle_call({:exists?, key}, _from, opts) do response = apply(opts.backend_module, :exists?, [ diff --git a/lib/cacheman/backend/redis.ex b/lib/cacheman/backend/redis.ex index 4c77692..2dd6f1a 100644 --- a/lib/cacheman/backend/redis.ex +++ b/lib/cacheman/backend/redis.ex @@ -20,6 +20,17 @@ defmodule Cacheman.Backend.Redis do end) end + def get_batch(conn, keys) when is_list(keys) do + list_of_commands = + Enum.map(keys, fn key -> + ["GET", key] + end) + + :poolboy.transaction(conn, fn c -> + Redix.pipeline(c, list_of_commands) + end) + end + def exists?(conn, key) do :poolboy.transaction(conn, fn c -> case Redix.command(c, ["EXISTS", key]) do diff --git a/test/cacheman_test.exs b/test/cacheman_test.exs index d6a6ab9..601d7c0 100644 --- a/test/cacheman_test.exs +++ b/test/cacheman_test.exs @@ -72,17 +72,13 @@ defmodule CachemanTest do assert {:ok, nil} = Cacheman.get(:good, key2) end - test "put_batch cant reach redis srever" do + test "get_batch" do key1 = "test-#{:rand.uniform(10_000)}" key2 = "test-#{:rand.uniform(10_000)}" - assert {:ok, nil} = Cacheman.get(:good, key1) - assert {:ok, nil} = Cacheman.get(:good, key2) - - assert {:error, _} = Cacheman.put_batch(:broken, [{key1, "value1"}, {key2, "value2"}]) + assert {:ok, 2} = Cacheman.put_batch(:good, [{key1, "value1"}, {key2, "value2"}]) - assert {:ok, nil} = Cacheman.get(:good, key1) - assert {:ok, nil} = Cacheman.get(:good, key2) + assert {:ok, ["value1", "value2"]} = Cacheman.get_batch(:good, [key1, key2]) end test "fetch and store" do @@ -162,6 +158,26 @@ defmodule CachemanTest do assert {:ok, nil} = Cacheman.get(:broken, "test1") end + test "put_batch cant reach redis srever" do + key1 = "test-#{:rand.uniform(10_000)}" + key2 = "test-#{:rand.uniform(10_000)}" + + assert {:ok, nil} = Cacheman.get(:good, key1) + assert {:ok, nil} = Cacheman.get(:good, key2) + + assert {:error, _} = Cacheman.put_batch(:broken, [{key1, "value1"}, {key2, "value2"}]) + + assert {:ok, nil} = Cacheman.get(:good, key1) + assert {:ok, nil} = Cacheman.get(:good, key2) + end + + test "get_batch" do + key1 = "test-#{:rand.uniform(10_000)}" + key2 = "test-#{:rand.uniform(10_000)}" + + assert {:ok, [nil, nil]} = Cacheman.get_batch(:broken, [key1, key2]) + end + test "fetch and store" do key = "test-#{:rand.uniform(10_000)}" From 2fcce2126f414beda52dfba681edca969d336925 Mon Sep 17 00:00:00 2001 From: VeljkoMaksimovic Date: Thu, 4 Aug 2022 16:13:16 +0200 Subject: [PATCH 4/5] Fix pattern matching in put_batch --- lib/cacheman.ex | 6 +++--- lib/cacheman/backend/redis.ex | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/cacheman.ex b/lib/cacheman.ex index f14e64e..3495e8c 100644 --- a/lib/cacheman.ex +++ b/lib/cacheman.ex @@ -309,10 +309,10 @@ defmodule Cacheman do response = apply(opts.backend_module, :put_batch, [ opts.backend_pid, - Enum.map(key_value_pairs, fn key_value_pair -> + Enum.map(key_value_pairs, fn {key, value} -> { - fully_qualified_key_name(opts, elem(key_value_pair, 0)), - elem(key_value_pair, 1) + fully_qualified_key_name(opts, key), + value } end), put_opts diff --git a/lib/cacheman/backend/redis.ex b/lib/cacheman/backend/redis.ex index 2dd6f1a..794e00b 100644 --- a/lib/cacheman/backend/redis.ex +++ b/lib/cacheman/backend/redis.ex @@ -52,8 +52,8 @@ defmodule Cacheman.Backend.Redis do def put_batch(conn, key_value_pairs, ttl) when is_list(key_value_pairs) do list_of_commands = - Enum.map(key_value_pairs, fn pair -> - ["SET", elem(pair, 0), elem(pair, 1)] ++ ttl_command(ttl) + Enum.map(key_value_pairs, fn {key, value} -> + ["SET", key, value] ++ ttl_command(ttl) end) :poolboy.transaction(conn, fn c -> From 4668defdb6bf19b2cad0eba584a8e087b655ce34 Mon Sep 17 00:00:00 2001 From: VeljkoMaksimovic Date: Fri, 5 Aug 2022 11:14:00 +0200 Subject: [PATCH 5/5] Fix typo --- test/cacheman_test.exs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/cacheman_test.exs b/test/cacheman_test.exs index 601d7c0..4016bca 100644 --- a/test/cacheman_test.exs +++ b/test/cacheman_test.exs @@ -158,7 +158,7 @@ defmodule CachemanTest do assert {:ok, nil} = Cacheman.get(:broken, "test1") end - test "put_batch cant reach redis srever" do + test "put_batch cant reach redis server" do key1 = "test-#{:rand.uniform(10_000)}" key2 = "test-#{:rand.uniform(10_000)}"