Support caching nil values
#16
Conversation
| @@ -278,6 +270,17 @@ defmodule Mentat do | |||
| Supervisor.stop(name) | |||
| end | |||
|
|
|||
| defp lookup(cache, key) do | |||
There was a problem hiding this comment.
Initially, I copy/pasted the implementation from get/2 into fetch/4 and tweaked it like this, because it didn't have the information it needed just by calling get like it was doing before. Then, I extracted the result here because I thought it worked well for both and also simplified the implementation of get/2
|
The GHA workflows need updating so tests will actually run, the Elixir and OTP versions are far too old. Other than that seems fine, will wait for @keathley to take a look. |
lib/mentat.ex
Outdated
| val | ||
| end | ||
| {status, value} = lookup(cache, key) | ||
| :telemetry.execute([:mentat, :get], %{status: status}, %{key: key, cache: cache}) |
There was a problem hiding this comment.
looks like the telemetry could stay in the case statement for lookup/2?
There was a problem hiding this comment.
Aha you're totally right! I'll fix that while it's top of mind before we merge this.
|
@mhanberg I'm good with this. |
|
Unrelated thought: Looking at the telemetry, I'm wondering why we never added span events to mentat. Seems like an easy enough change to make and might have surfaced this issue sooner. Either way maybe that would be a nice thing to do at some point in the future. Not super necessary. Just a nice to have. |
Yeah I had the same thought recently while cleaning up some internal metrics / dashboards that used to have cache read latency in them and was like "huh I wonder how we never noticed that we didn't have this?" My guess is just that it's usually fast when it hits, so we mainly only care about the hit to miss ratio, but you're right that it would be easy to add and maybe I will do it if someone else doesn't get to it first! |
Since
Mentat.fetchspecifically requires that the fallback function return{:commit, value}or{:ignore, value}, anilshould be able to be stored in cache and not be treated as a miss whenfetchis called again later.This bug went unnoticed for so long because it often behaves correctly if the intended value was
nil, because the fallback function would probably also end up returningnilif called again later. However, this is very inefficient compared to actually caching and returning anilvalue in this case and it also makes the metrics incorrect, firing a[:mentat, :miss]even though it actually had anilvalue stored.This change fixes those issues and allows a
nilto be stored and retrieved from ETS just like any other value.