Create a simple cache #4

ankhers · 2024-02-21T16:58:55Z

This will prevent the need from retrieving the value for each and every call. This may not be overly important for env vars that are currently implemented. However, this could become a big cost saver for when you are retrieving your values from an external service.

We also renamed the MyConfig.validate!/0 function to MyConfig.load!/0. You are now required to call MyConfig.load!/0 before you try using your configuration as that is what will actually grab the values and store them into the cache.

This work is based off of #2.

This will prevent the need from retrieving the value for each and every call. This may not be overly important for env vars that are currently implemented. However, this could become a big cost saver for when you are retrieving your values from an external service. We also renamed the MyConfig.validate!/0 function to MyConfig.load!/0. You are now required to call `MyConfig.load!/0` before you try using your configuration as that is what will actually grab the values and store them into the cache.

sasa1977 · 2025-02-04T14:26:26Z

lib/provider.ex

+            # Provider.fetch_one!(
+            #   unquote(Keyword.fetch!(spec, :source)),
+            #   unquote(param_name),
+            #   unquote(param_spec)
+            # )


This looks like a leftover

sasa1977 · 2025-02-04T14:39:32Z

lib/cache.ex

+  @spec set(module(), atom(), term()) :: :ok
+  def set(mod, key, value) do
+    impl().set(mod, key, value)
+  end
+
+  @spec get(module(), atom()) :: {:ok, term()} | {:error, :not_found}
+  def get(mod, key) do
+    impl().get(mod, key)
+  end
+
+  defp impl do
+    Application.get_env(:provider, :cache) || Provider.Cache.ETS
+  end


I'm not sold on this polymorphism. I feel it adds complexity to the code with no real benefits. I think that just using ETS directly should be enough. Basically, you invoke Provider.fetch_all and then store all k-v pairs at once using :ets.insert(table, objects).

Another thing that could be done is to make the GenServer be the client module. So if I have something like:

defmodule MyConfig do use Provider # ... end

I can inject MyConfig as a child spec. This GenServer would start the ETS table with the same name as the module. It would also prime the cache during init, failing to start if parameters fail the validation. What do you think?

I think this change would greatly simplify the API. I think the main question from me would be, if we were to do something like this, is there a point in exposing the validate!/0 and fetch_all/0 functions? With this proposed change, I don't think they need to be public anymore and can just be part of the internals.

We would also be able to provide a configuration option that would allow us to set how frequently, if at all, we would want to refresh the cache.

defmodule MyConfig do use Provider, cache_refresh_time: 10_000 end

or something. This way we do not need to do something like :timer.apply_interval/2 to continually refresh the cache.

Ultimately this would mean that the only public functions to this module would become

start_link/1 to get this into our application supervisor

init/1 to facilitate being a GenServer

handle_call/3 / handle_cast/2 to allow for the refresh mechanism

The functions we generate based on the passed in configuration (e.g., db_host/0, etc)

Is this in line with what you were thinking?

is there a point in exposing the validate!/0 and fetch_all/0 functions?

These functions are already removed with your current changes :-) However, there's the new load! function, and it should IMO remain to allow client code to refresh the cache when it wants to.

I wouldn't bother with supporting periodic refreshing. It's really easy to add that in the client code. But more importantly, I don't think I'd want to periodically refresh in the client app. More likely I'd have some message sent to some process when config changes, and then I'd invoke load!. Or I'd periodically poll the source to check if there are changes, but I wouldn't refresh unless something has changed.

Ultimately this would mean that the only public functions to this module would become

The public API would be: start_link, child_spec, load!, and the generated getters. I don't consider the callback funs to be the public API, they are callbacks invoked by gen_server, not by the client code. And being marked with @impl they will be also excluded from docs.

sasa1977 · 2025-02-04T14:42:39Z

lib/provider.ex

+               }
+             ) do
+          {:ok, values} ->
+            Enum.each(values, fn {k, v} -> Provider.Cache.set(__MODULE__, k, v) end)


You can insert all entries at once using :ets.insert/2

sasa1977 · 2025-02-04T14:43:28Z

lib/cache/ets.ex

+
+  @impl Provider.Cache
+  def set(module, key, value) do
+    GenServer.call(__MODULE__, {:set, module, key, value})


Don't think this needs to go through the GenServer.

Unless my understanding is incorrect, we would need to enable write concurrency to not put this through the GenServer. Though, ideally, only a single process would ever update this anyways, so maybe it is not an issue? But I think this issue also disappears with your other comment of making the config module also a GenServer.

According to docs:

The entire operation is guaranteed to be atomic and isolated, even when a list of objects is inserted.

So you're safe running this from multiple processes. I don't think write concurrency is needed here, because there won't be multiple simultaneous writes of different rows. We always store everything, and that's done with a single atomic & isolated ETS operation.

Needed to make the table :public in order for that to work.

Yeah, forgot about that. I think that's fine.

sasa1977 · 2025-02-04T14:44:18Z

lib/cache/ets.ex

+
+  @impl GenServer
+  def init(:ok) do
+    state = :ets.new(__MODULE__, [:named_table])


It's probably worth turning :read_concurrency for this table, as it will be mostly used for (possibly frequent) reads by many processes.

The polymorphism did not really add anything to the implementation and made it messier than it needed to be. This change also turned the config module into a GenServer that will start a named ETS table (the cache) and load all of the data from the source into the cache for quick retrieval. Currently, the only purpose of the GenServer is to own the ETS table.

ankhers · 2025-02-04T20:50:14Z

test/provider_test.exs

      assert TestModule.opt_7() == 3.14
-    end

-    test "access function raises for on error" do


This test is no longer applicable since we are expecting someone to call MyConfig.start_link() before attempting to use the values. And if we did that before the test like we did in other places, we would get an error at that point.

sasa1977

Had a few nits, and those are optional. The only thing that you really need to change is switching to start_supervised!. You could also consider wrapping that in a private helper fun, e.g. start_config_server! or something like that, but that's also optional.

sasa1977 · 2025-02-05T09:22:37Z

lib/provider.ex

+      def child_spec(_arg) do
+        %{
+          id: __MODULE__,
+          start: {__MODULE__, :start_link, []}
+        }


Nit: use GenServer already generates child_spec, the only diff is that it invokes start_link/1. So if you had something like:

def start_link(arg), do: GenServer.start_link(__MODULE__, arg, ...) def init(_arg), do: ...

Then you wouldn't need to define child_spec yourself.

sasa1977 · 2025-02-05T09:23:05Z

lib/provider.ex

+        Provider.Cache.new(__MODULE__)
+
+        load!()
+
+        {:ok, nil}


Nit: perhaps the blank lines are needless with this small amount of code

sasa1977 · 2025-02-05T09:26:10Z

lib/provider.ex

             ) do
          {:ok, values} ->
-            Enum.each(values, fn {k, v} -> Provider.Cache.set(__MODULE__, k, v) end)
+            Provider.Cache.set(__MODULE__, Enum.to_list(values))


Nit: since values is a map, it would be slightly better to use Map.to_list, to make things a bit clearer.

sasa1977 · 2025-02-05T09:31:31Z

test/provider_test.exs

      System.put_env("OPT_6", "false")
      System.put_env("OPT_7", "3.14")

+      {:ok, pid} = TestModule.start_link()


Do this instead

pid = start_supervised!(TestModule, restart: :temporary)

And you won't need to manually stop the server. As the docs say:

The advantage of starting a process under the test supervisor is that it is guaranteed to exit before the next test starts.

sasa1977 · 2025-02-05T09:33:48Z

test/provider_test.exs

+
      assert TestModule.load!() == :ok
+
+      GenServer.stop(pid)


Also, this is a bit unreliable, because if the assertion fail, this won't execute. Now, the server will still be stopped, but it happens asynchronously, so it might happen while the next test is running. And because all of the servers use the same registered name, the next test might fail.

This is going to be more reliable that manually starting and stopping the GenServer. Mainly because if a test fails, the call to stop the GenServer will not be made, resulting in the server not stopping and potentially making other tests fail when they should not.

When writing the spec for Provider.Cache.get/2, I realized that the error case is not something that should ever happen anymore. One of two things should happen when calling that function. Either, * The user forgot to start the GenServer, in which case ets will complains that the table does not exist; or * The user did start the GenServer, in which case the ets table exists and it will be initialized with the data from the configured source. And if you happen to not give a default to a given param and forget to configure it, the GenServer will fail to start because of it.

sasa1977 · 2025-02-05T16:25:23Z

lib/provider/cache.ex

+  """
+
+  @spec new(atom()) :: :ok
+  def(new(name)) do


What's with this formatting?

I think that happened after I partially typed out the spec and saved the file and that is what happened when mix formatted it... I'll fix that.

sasa1977 · 2025-02-05T16:28:54Z

lib/provider/cache.ex

@@ -0,0 +1,27 @@
+defmodule Provider.Cache do


Not insisting, but given the amount of code in this module, and the fact that each function is called exactly once from the single client module, I think this would have better worked as just inline :ets calls in the client module.

That is very valid. I think I will just put those calls in the provider module.

ankhers mentioned this pull request Feb 26, 2024

Json endpoint #5

Closed

sasa1977 reviewed Feb 4, 2025

View reviewed changes

ankhers commented Feb 4, 2025

View reviewed changes

sasa1977 reviewed Feb 5, 2025

View reviewed changes

ankhers added 2 commits February 5, 2025 06:20

sasa1977 reviewed Feb 5, 2025

View reviewed changes

Clean up the cache interface

4faf5db

sasa1977 merged commit ea02a89 into ankhers:main Feb 5, 2025
1 check passed

Create a simple cache #4

Create a simple cache #4

Uh oh!

Conversation

ankhers commented Feb 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sasa1977 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants