-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Move -epmd_module and -erl_epmd_port into kernel parameters #8671
Conversation
CT Test Results 5 files 206 suites 1h 58m 18s ⏱️ Results for commit 1714f2f. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts// Erlang/OTP Github Action Bot |
_:_ -> | ||
case net_kernel:epmd_module() of | ||
erl_epmd -> | ||
case erl_epmd:listen_port_please(nonode, nohost) of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note the fake nonode
, nohost
, those arguments are ignored by erl_epmd:listen_port_please/2
, and this way we use the public API. Alternatively we could expose erl_epmd:erl_epmd_listen_port/0
and make it -doc false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also expose it as net_kernel:epmd_listen_port
, to mirror net_kernel:epmd_module
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
epmd_module
is a broader config, while erl_epmd_listen_port
is specific to erl_epmd
, as per the other comment.
A follow up question, if this change is accepted, would it be possible to have it in the next maintenance release? If yes, I will change the target branch, and then I can submit a separate PR altering the |
Thanks for the PR! I think we'll want to discuss this a bit more internally before merging, and most of us are on vacation right now. We'll revisit this later in the summer/early autumn. :-) |
@jhogberg sounds good, thanks for the heads up! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comments are from the OTP team (not just me, so credit to them for this review)
Please, address the issues mentioned and feel free to rebase on top of maint
.
102dfda
to
d0004eb
Compare
Co-authored-by: Lukas Larsson <[email protected]>
If there is nothing left to fix, I will merge this next Monday (some of us are in a conference this week and I do not want to rush the merging before having a really final good look at it :) ) |
@kikofernandez sounds good, thanks :) |
@kikofernandez kindly ping :) |
We have a code freeze for 27.1. I will merge as soon as I am given the OK. |
Merging tomorrow if no internal tests break |
@kikofernandez thank you :) |
This introduces
epmd_module
anderl_epmd_listen_port
parameters for the kernel application. I updated the docs to deprecate-epmd_module
and-erl_epmd_port
, similarly to-connect_all
in 1d0fe6f.The kernel application already has a lot of parameters related to distribution, so I believe semantically this makes sense.
For context, in Livebook we started using a custom
epmd_module
. In Escript and Elixir releases, there are ways to set the-epmd_module
flag, however there is no way to do that in development, other than settingERL_AFLAGS
env var. We already start distribution manually usingnet_kernel:start/2
, so the idea is that now we could doapplication:set_env(kernel, epmd_module, Module)
right before that.