Allow beeline to match Elixir's OTP naming #8
Conversation
GenServer's in elixir can be one of several type structures. https://hexdocs.pm/elixir/GenServer.html#t:name/0 The current implementation of Beeline only works with atoms (which match to OTP's `{:local, name}` for named processes, but this limits the places BeeLine can be used as it either leaks atoms or prevents from having more ant one GenServer of the same name running at a single time. By updating the naming to include the global and via options the user can decide how to control this instead of the library.
|
As a note on the mix.exs updates. This does require |
pmonson711
requested review from
the-mikedavis,
electricshaman and
tonyvanriet
lib/beeline/process_naming.ex
Outdated
| {:global, Module.concat(base_name, appended_name)} | ||
| end | ||
|
|
||
| def name({:via, registry, {registry_name, base_name}}, appended_name) do |
There was a problem hiding this comment.
I just want to call out that this {:via...} pattern may not be complete. It will work with elixir's Registry module but it may not work with other common erlang registries like gproc.
|
There was a similar change in Broadway for allowing via tuples: dashbitco/broadway#239. They don't support global tuples but I don't think they're intentionally preventing them. |
|
It's interesting the approach Broadway takes with the overridable and no default implementation for the name mixed with a guard. That could work for Beeline as well but it seems like it might cause confusion in the config and the existing |
pmonson711
force-pushed
the
allow-dynamic-named-processes
branch
from
ddbcc4d
to
fb7d906
Compare
pmonson711
force-pushed
the
allow-dynamic-named-processes
branch
from
fb7d906
to
64515b7
Compare
|
I added a guard and updated the allowable names to match. It's slightly different than before. Global names now match the OTP type where the second element of the tuple is a term. Registry names are now limited to Elixir's Registry module, because that's all we test, and matches what Broadway does in their PR. |
| @@ -3,14 +3,13 @@ defmodule Beeline.Topology do | |||
|
|
|||
| @behaviour GenServer | |||
|
|
|||
| import Beeline.ProcessNaming, only: [name: 2] | |||
| alias __MODULE__.StageSupervisor | |||
|
|
|||
| defstruct [:supervisor_pid, :config] | |||
|
|
|||
| def start_link(config) do | |||
There was a problem hiding this comment.
If this were start_link(config, options \\ []) we could just pass options directly to GenServer. Then, from another higher level module in your API, write the layer which handles the process name shenanigans. I feel a soapbox rant coming on.
There was a problem hiding this comment.
This is my first time ever seeing this library, so I don't have a lot of context built up about its design or usage. Best to mention that up front.
Your changes seem totally reasonable as an expansion of what's already here.
Beyond that, my unpopular thoughts: I don't understand the need for all of the ceremony around the naming of processes in the first place. I think it's admirable to want to hide the complexity of the supervision tree from clients, because honestly it's tedious and can be annoying to deal with, but in reality the people who will be using this library are already likely to know how to spin this up in their own application's supervision tree. This library seems to jump through a lot of hoops just to keep the Beeline API from exposing the process names and the supervision tree of stages. There's something to be said for that, but personally I find libraries like that frustrating to work with because they make assumptions about how I want to use OTP in my application and often times they restrict one's normal options like name and debug: [:trace].
|
The more I look over Beeline, the more I think the above feedback is not really applicable here. Basically old man yelling at cloud situation. |
GenServer's in elixir can be one of several type structures.
https://hexdocs.pm/elixir/GenServer.html#t:name/0
The current implementation of Beeline only works with atoms (which match
to OTP's
{:local, name}for named processes, but this limits theplaces BeeLine can be used as it either leaks atoms or prevents from
having more than one GenServer of the same name running at a single time.
By updating the naming to include the global and via options the user
can decide how to control this instead of the library.