search

whitfin / local-cluster

Easy local cluster creation for Elixir to aid in unit testing
MIT License
223 stars 30 forks source link

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

Closed JesseStimpson closed 1 month ago

JesseStimpson commented 1 month ago

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.

whitfin commented 1 month ago

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 commented 1 month ago

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

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

whitfin commented 1 month ago

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!

JesseStimpson commented 1 month ago

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

whitfin commented 1 month ago

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