Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include app name in :pg group namespace #1065

Closed

Conversation

warmwaffles
Copy link

We are running 4 separate elixir applications in the same cluster. Each application talks to each other using :pg. But each application has its own Oban instance running. We were running into issues where other nodes were showing up in the dashboard.

I tried setting just name: MyApp.Oban and on the dashboard router helper oban_name: MyApp.Oban. This did not work. Locally I made the following changes in my deps/ directory, recompiled and this seems to do the trick.

If you have a better solution, feel free to close this. I just wanted to bring this up as potential solution.

@warmwaffles
Copy link
Author

I'm unsure if there are tests for this in the test directory.

@@ -50,7 +50,7 @@ defmodule Oban.Notifiers.PG do
@impl Notifier
def notify(server, channel, payload) do
with %{conf: conf} <- get_state(server) do
pids = :pg.get_members(__MODULE__, conf.prefix)
pids = :pg.get_members(__MODULE__, {conf.name, conf.prefix})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be a breaking change for any app that relies on connecting different apps that share an oban_jobs table. Instead, I think the solution is to pass a namespace option through and have it fall back to conf.prefix.

In application config it would look like this:

notifier: {Oban.Notifiers.PG, namespace: {Oban, "public"}}

WDYT?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Funny enough @mbaleczny suggested something like that as well.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sorentwo I don't see how this would effect two apps connected to the same database using the same table. If both have the same name: then they will still communicate as normal but if the name: is different, then they won't communicate.

Either way, I do agree that this should probably be a configuration done with the :notifier. I don't know where all of that would need to change as my understanding is that :notifier is expected to be a module only.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how this would effect two apps connected to the same database using the same table. If both have the same name: then they will still communicate as normal but if the name: is different, then they won't communicate.

That's the issue—if people are relying on the fact that two instances with different names (Oban.A and Oban.B) can communicate, then this would break their app.

I don't know where all of that would need to change as my understanding is that :notifier is expected to be a module only.

I can make the changes. It will look roughly like https://github.com/sorentwo/oban_notifiers_phoenix 🙂

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I'll close this PR then.

sorentwo added a commit that referenced this pull request Apr 10, 2024
By default, all Oban instances using the same `prefix` option would
receive notifications from each other. Now you can use the `namespace`
option to separate instances that are in the same cluster _without_
changing the `prefix`.

Addresses #1065
@sorentwo
Copy link
Member

@warmwaffles Done! See the linked commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants