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
new P2P info message #3423
new P2P info message #3423
Conversation
Corresponding protocol PR will follow |
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.
Questionmark regarding the apparently hard-coded numbers for verified/unverified peers. Otherwise LGTM.
NodeInfo = #{ version => NodeVersion | ||
, os => OS | ||
, network_id => aec_governance:get_network_id() | ||
, verified_peers => 2 |
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.
??
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.
Oops
apps/aecore/src/aec_sync.erl
Outdated
end) | ||
end, | ||
L), | ||
pmap_gather(length(L), []). |
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.
One way to write such a pmap (apologies for syntax errors):
Pids = [spawn_monitor(fun() -> exit({ok, PerformTask(E)} end) || E <- L],
pmap_gather(Pids, []).
pmap_gather([], Acc) ->
Acc;
pmap_gather([{Pid, MRef}|Pids], Acc) ->
receive
{'DOWN', MRef, process, Pid, Res} ->
case Res of
{ok, GoodRes} -> pmap_gather(Pids, [GoodRes | Acc]);
Error -> do_something_clever()
end
end.
This also captures crashes, and makes it possible to tell the difference.
Co-authored-by: Ulf Wiger <ulf@wiger.net>
@@ -50,6 +52,15 @@ get_generation(PeerId, Hash) -> | |||
set_last_generation_in_sync() -> | |||
gen_server:cast(?MODULE, set_last_generation_in_sync). | |||
|
|||
ask_all_for_node_info() -> |
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.
I think it should be exposed by HTTP for internal use.
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.
I wouldn't make this part of the node at all, it's a reporting functionality IMO.
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.
I'd rather keep it as an internal API which we call on demand. My concern would be an abuse of this API.
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 version in the info msg can get out of sync with the real version.
Fixes #3107
The work on this PR had been supported by the Aeternity Foundation.