cpursley / walex

Postgres change events (CDC) in Elixir
MIT License
276 stars 14 forks source link

Supervisor process foundable with :via option #44

Closed DohanKim closed 7 months ago

DohanKim commented 7 months ago

(This PR is stacked with the previous one)

The current implementation of WalEx.Config.Registry

  def set_name(:set_supervisor, module, app_name), do: {:via, module, app_name}

makes the supervisor process can’t be found with GenServer.whereis as WalEx.Supervisor is not implementing Registry behavior

test:

      assert {:ok, walex_supervisor_pid} = WalExSupervisor.start_link(@base_configs)

      assert walex_supervisor_pid ==
               GenServer.whereis(
                 WalExRegistry.set_name(:set_supervisor, WalExSupervisor, :test_name)
               )

produces the error

** (UndefinedFunctionError) function WalEx.Supervisor.whereis_name/1 is undefined or private
    (walex 3.7.0) WalEx.Supervisor.whereis_name(Sendman)
    (elixir 1.16.0) lib/gen_server.ex:1286: GenServer.whereis/1
    (elixir 1.16.0) lib/gen_server.ex:1059: GenServer.stop/3

So I changed the WalEx.Config.Registry to use

  def set_name(:set_supervisor, module, app_name), do: set_name(module, app_name)

like other modules

also,

    Supervisor.start_link(__MODULE__, configs: supervisor_opts, name: name)

doesn’t properly register the process name (it is using start_link/2) We can check this with the test case above.

So I changed to use start_link/3

Supervisor.start_link(__MODULE__, supervisor_opts, name: name)

With this change, each Supervisor is starting with the name attached and also foundable with its name

DohanKim commented 7 months ago

also changed other supervisors to use start_link/3 to attach the name properly

DohanKim commented 7 months ago

fixing the test failure

DohanKim commented 7 months ago

there was also a bug setting WalEx.Destinations.Supervisor name reading :app_name key instead of :name

cpursley commented 7 months ago

Very much appreciated @DohanKim

cpursley commented 7 months ago

@DohanKim I just created a new release: https://github.com/cpursley/walex/releases/tag/v3.8.0

Also, does this finally resolve https://github.com/cpursley/walex/issues/38 or are you still experiencing issues?

DohanKim commented 7 months ago

I think it would, but as I can't reproduce the problem on dev or test environment, I have deployed the PR on my prod server and watching if it's okay. I can say it's resolved whenever a temporary DB connection problem happens and WalEx recovers the connection successfully.

cpursley commented 7 months ago

Okay, feel free to close the issue whenever you feel confident that it's resolved :)