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

Supports :peer module (>= OTP 25) #30

Merged
merged 1 commit into from
Aug 25, 2024
Merged

Conversation

JesseStimpson
Copy link
Contributor

Hello, it appears that the PR for :peer has become stagnant (#28). Will you consider this in its place? Thanks!

API change: LocalCluster.start_nodes/3 no longer returns node names. See LocalCluster.nodes/1.

API change: LocalCluster.start_nodes/3 no longer returns node
names. See LocalCluster.nodes/1.
@whitfin
Copy link
Owner

whitfin commented Aug 25, 2024

Hi @JesseStimpson!

Funny timing, I was just about to work on the other PR today and get it cleaned up as I finally have some OSS time. After review I think I'd like your thoughts on some changes. Whether you want to tackle them in this PR is up to you!

I'm trying to think of a better API design here. I wrote this several years ago, and I think it could be improved. What do you think about this (roughly):

# this is the new API we recommend in docs
def start_link(prefix, amount, options \\ []) do
  # start a supervisor which spawns all peers underneath it, and return supervisor pid
end

# we would probably also add `child_spec/1`

# similar to what you have
def nodes(pid) do
  # list the nodes; we can infer this from the supervision tree
  # there's no need to have pids/1 as it's in the supervision tree
end

# backwards compatible, with a @deprecated tag
def start_nodes(prefix, amount, options \\ []) do
  {:ok, pid} = start_link(prefix, amount, options)
  nodes(pid)
end

How does this sound? We could push this out in a minor and have people begin to migrate to the new setup, then strip out the old later in a major bump (likely when we remove :slave completely).

I totally understand this is more than you intended to contribute, so I'm happy to take it over if you prefer! I can merge what you have and map to this new approach if you agree that it's better.

Edit: don't worry about the Elixir < 1.7 failures. Updates to the rebar setup dropped compatibility, so we'll do the same here. So maybe this can be a major regardless, although I'd still keep start_nodes/3 to reduce churn, I think.

@JesseStimpson
Copy link
Contributor Author

Ah didn't mean to step on your toes. 😄

Your suggestions sound great, and I'm happy to have a go at them.

@whitfin
Copy link
Owner

whitfin commented Aug 25, 2024

Ah didn't mean to step on your toes.

Not a problem! Just funny how I haven't touched this in months and it happens to be the exact same day 😅

I've been looking at it all for the last hour or so and I think it's probably something I should play around with rather than waste your time on a possible dead end. So I'm going to merge this PR and then update the build.

Thanks for your changes, have a great weekend!

@whitfin whitfin merged commit bc69c48 into whitfin:main Aug 25, 2024
11 of 13 checks passed
@JesseStimpson
Copy link
Contributor Author

Hi @whitfin , just wanted to let you know I'm using 2.0 now. Thanks for your work on this. 😄

@whitfin
Copy link
Owner

whitfin commented Aug 26, 2024

@JesseStimpson awesome! Let me know if you encounter any problems and I'll get them fixed up ASAP 🎉

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.

2 participants