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

Using via_tuple as name #236

Closed
acco opened this issue May 25, 2021 · 8 comments
Closed

Using via_tuple as name #236

acco opened this issue May 25, 2021 · 8 comments

Comments

@acco
Copy link
Contributor

acco commented May 25, 2021

Hey team,

Great project :) I want to use a via_tuple for my Broadway server's name. This is because we'll be spinning up many pipelines. But running an issue because name is enforced to be an atom:

https://github.com/dashbitco/broadway/blob/master/lib/broadway/options.ex#L6-L13

Am I missing something? / would a via_tuple be desirable to support here?

Thanks,
Anthony

@josevalim
Copy link
Member

It is doable but not supported right now because we name each individual process in the Broadway tree. So if we want to support something similar, we probably need a def naming function in the topology that receives an atom and returns either an atom or a via name. This way we will call this function for every process we have to name. A PR is welcome!

@acco
Copy link
Contributor Author

acco commented May 26, 2021

Got it @josevalim. To make sure I'm following, for the PR I assume I'd address all the call sites for config.name inside topology.ex?

@josevalim
Copy link
Member

Yes, and the places we build the names after config.name. Yeah!

@acco
Copy link
Contributor Author

acco commented May 27, 2021

Hey @josevalim – started hacking at this. Issue I ran into: a via tuple can take most any form, right? Because the generic form is this:

{:via, Registry, {custom_registry_name, args_forwarded_to_custom_registry}}

Therefore these are both valid via tuples:

{:via, Registry, {MyRegistry, MyBroadway}}
{:via, Registry, {MyRegistry, {MyBroadway, "some-id"}}}

This flexibility presents a challenge because presumably we'd want to use via tuples to name the processes in the rest of the topology. But if args_forwarded_to_custom_registry can take any shape, generating via tuples based on a via tuple sounds like trouble.

Re-reading your comment, maybe you're suggesting that we allow the naming function to be an option you can pass in to start_link to override default naming scheme?

id = "1-asdf"
Broadway.start_link(Forwarder,
          name: MyRegistry.via_tuple({Forwarder, id}),
          naming: fn base_name -> MyRegistry.via_tuple({:"Forwarder.#{base_name}", id}) end
          # ...
        )

Another possibility is I'm barking up the wrong tree and I don't need a via tuple. I'll be starting lots of Broadway pipelines, so I want their names to be dynamic. In the past with GenServers, I've used via tuples in this situation that included a UUID. Perhaps instead I can get away with naming these pipelines eg :"MyBroadway.#{hash(uuid)}"

Thanks!

@josevalim
Copy link
Member

The problem with generating atoms like that is that you will eventually exhaust the atom table. So if you do that based on user input, you will have an issue.

@josevalim
Copy link
Member

However, if you do it based on another value which is bound, say a predefined number of Kafka topics, then that’s fine.

@acco
Copy link
Contributor Author

acco commented May 27, 2021

Thanks. I can get by with just using atoms for the short-term, but can see how via tuple would be desirable in long run.

I just realized we could have the pipeline module that invokes use Broadway define a function. So instead of passing it in as an opt to the pipeline, something like this would override the default naming function:

defmodule MyPipeline
  use Broadway

  def process_name(base_name, suffix) do
    # Called to name every process in the topology. Return an atom or a via tuple.
  end
end

Do you like this approach or should we consider something else?

@josevalim
Copy link
Member

@acco yes! that's the approach i originally had in mind :) but the approach with the function was equivalent enough.

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

No branches or pull requests

2 participants