Restructure alerts engine to merge new results into cache instead of delete/write #864
Conversation
lib/engine/ets.ex
Outdated
| @@ -0,0 +1,34 @@ | |||
| defmodule Engine.ETS do | |||
There was a problem hiding this comment.
Also thought about naming this module Engine.Cache since ETS is acting as our cache.
There was a problem hiding this comment.
Hmm yeah interesting thought - it's true that ETS essentially acts as our cache, although I'm not sure how I feel about abstracting it as such. It could help the cleanliness of some of the engine logic if we abstracted away the direct calls to :ets I suppose. Curious to see if @panentheos has thoughts
There was a problem hiding this comment.
I agree with Paul, I'm not sure there's much to be gained from adding new terminology here. Also, we usually use the name Engine to describe GenServer processes that are actively doing something, like polling data repeatedly. Since these are just functions, I'd probably file them under something with "Utilities" in the name, or just add them to one of the existing utility modules if they fit there.
There was a problem hiding this comment.
Gotcha, that distinction for engines makes sense to me! Given the other common use cases you pointed out, I think having this as an ETSUtilities module works well
|
While the delete/insert sequence in the original code is indeed error-prone, ETS does guarantee atomicity for several single operations, including bulk inserts. We leverage that to avoid the problem elsewhere by defining a sentinel "empty" value, overwriting all old values with that, merging the new values, and then writing the whole thing back in one operation. There's been a bit of consolidation of the code for this, and this would be a good opportunity to finish that up. See |
|
@panentheos I did pull out the implementations into a shared utility module, but the behavior of the predictions/locations ETS table keys is different than what we want for alerts. Because of that, I kept them as two separate utility functions. I thought about combining them into one with two methods based on the number of inputs, but I ultimately decided that ended up being more complex. |
| @@ -9,6 +9,7 @@ Here's a general overview of the realtime_signs application which should be help | |||
|
|
|||
| ## Development | |||
|
|
|||
| * If it's your first time using asdf, run `asdf plugin add erlang && asdf plugin add elixir`. | |||
There was a problem hiding this comment.
Thanks for keeping this updated and useful!
lib/signs/utilities/ets_utils.ex
Outdated
| and then merging new values into the table. | ||
| Existing values will be overwritten with the empty_value in ETS. | ||
| """ | ||
| def write_ets(table, values, empty_value) do |
There was a problem hiding this comment.
Since this is now more widely used, we could add a typespec to give some early warning if it's called incorrectly.
lib/signs/utilities/ets_utils.ex
Outdated
| end | ||
|
|
||
| @doc "Writes new key-values to table and removes any existing keys from ETS that aren't in new_entries." | ||
| def replace_contents(table, new_entry) when is_tuple(new_entry) do |
There was a problem hiding this comment.
Now that write_ets is available, we should be able to use that to solve our original problem without needing to add a new function. Looking at the alert code, we're already using :none as the default value when doing lookups, so if we use that as our "empty value" and set the old entries to that, it will have the same effect as deleting them.
There was a problem hiding this comment.
Makes sense -- I was thinking of saving space in ETS by deleting old keys, but I don't think that will be a large issue since most of them are re-used. Updated the code with this suggestion.
|
|
||
| ################################################################### | ||
| # write_ets tests | ||
| ################################################################### |
There was a problem hiding this comment.
FYI, you can use describe blocks to create logical groupings of tests, which can have their own locally-scoped setup blocks if desired.
There was a problem hiding this comment.
See ExUnit.Case.describe/2 for documentation on that! Also, when using this to group tests for different functions of a single module, it's conventional to use the function's name as the message. For example
describe "write_ets/3" do
test "does the thing" do
# ...
end
endThese are available unqualified in test modules because of use ExUnit.Case.
Summary of changes
Asana Ticket: Refactor RTS Alerts Engine
replace_allmethod for reusabilityReviewer Checklist
signs.jsonwas changed, there is a follow-up PR tosigns_uito update its dependency on this project