A draft of simplifying the service implementation #145
Conversation
| @@ -0,0 +1,56 @@ | |||
| defmodule EchoService do | |||
There was a problem hiding this comment.
I added a benchmark to try to measure the impact of these changes, but it turns out this branch only make a ~5% improvement in throughput.
We can get around 13.7k service messages per second on both the main branch and about 14.5k service messages per second on this branch. 🤷
|
|
||
| true -> | ||
| {:error, "You must provide a module or consuming function for the consumer supervisor"} | ||
| with {:ok, state} <- maybe_append_service(state, settings), |
There was a problem hiding this comment.
Curious if you'll like this change. The cond block was a bit tricky to read and I wanted to validate the user-input about the service definition during process startup rather than crashing the process later when the connection became available.
So I used a with block here to show the happy path, and I perform all of the service configuration validation right here (as well as initializing service counters etc). This way our stats will persist across re-connects and if the user has an invalid validation they'll get a helpful error at boot time rather than a process crash later
| @@ -189,35 +174,65 @@ defmodule Gnat.ConsumerSupervisor do | |||
| end | |||
| end | |||
|
|
|||
| defp initialize_as_manual_consumer(state, connection_pid) do | |||
| subscriptions = Enum.map(state.subscription_topics, fn(topic_and_queue_group) -> | |||
| defp subscribe_to_topics(%{service: service}, connection_pid) do | |||
There was a problem hiding this comment.
I generally prefer to do pattern matching in my function heads rather than if checks inside the function bodies. I don't feel strongly about this preference, but I like how it looks in this case. What do you think?
| {epname, groupname} | ||
| def maybe_respond(%{topic: topic} = message, service) do | ||
| case String.split(topic, ".") do | ||
| ["$SRV", @op_ping | rest] -> handle_ping(rest, service, message) |
There was a problem hiding this comment.
I liked pattern matching to grab the prefix, op and rest or tail this way rather than using List.slice. What do you think?
There was a problem hiding this comment.
yeah this looks much cleaner
There was a problem hiding this comment.
I don't know if there's anything that you want to incorporate from #144 , but this all looks better than what I had. It's been too long since I've written idiomatic Elixir.
|
I pulled in some of the typedoc and function doc improvements from #144 into this PR. Thanks for reviewing and sorry we ended tackling some of the same work. But I was glad to see how our thinking was converging. I'm on vacation so I suddenly had extra time for this project. I'll try to follow your lead on creating a github release |
After reviewing #141 a bit further and reviewing the ADR document, I had a few thoughts about how to simplify and optimize our service implementation:
:countersduring initialization and skip the use of ETSservicedatastructure to one module as possible. Use functions to pull out what is needed for service responder etc.ConsumerSupervisorNote: I wrote this before #144 was opened, but it looks like we were thinking of some of these same things