-
Notifications
You must be signed in to change notification settings - Fork 36
clear target in the group server when worker fails to connect to peer and reverts back to targeting #282
base: master
Are you sure you want to change the base?
clear target in the group server when worker fails to connect to peer and reverts back to targeting #282
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -147,6 +147,18 @@ handle_cast({add_handler, Key, Handler}, State=#state{handlers=Handlers}) -> | |
| {noreply, State#state{handlers=maps:put(Key, Handler, Handlers)}}; | ||
| handle_cast({request_target, inbound, WorkerPid, _Ref}, State=#state{}) -> | ||
| {noreply, stop_inbound_worker(WorkerPid, State)}; | ||
| handle_cast({clear_target, _Kind, _WorkerPid, Ref}, State=#state{workers = Workers}) -> | ||
| lager:debug("clearing target for worker ~p ", [_WorkerPid]), | ||
| %% the ref is stable across restarts, so use that as the lookup key | ||
| case lookup_worker(Ref, #worker.ref, State) of | ||
| Worker=#worker{} -> | ||
| %% TODO - should the pid be set to undefined along with target ? | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the pid would be set undefined here, the worker is still running.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed todo |
||
| NewWorkers = lists:keyreplace(Ref, #worker.ref, Workers, | ||
| Worker#worker{target=undefined}), | ||
| State#state{workers=NewWorkers}; | ||
| _ -> | ||
| State | ||
| end; | ||
| handle_cast({request_target, peerbook, WorkerPid, Ref}, State=#state{tid=TID}) -> | ||
| LocalAddr = libp2p_swarm:pubkey_bin(TID), | ||
| PeerList = case libp2p_swarm:peerbook(TID) of | ||
|
|
@@ -156,6 +168,7 @@ handle_cast({request_target, peerbook, WorkerPid, Ref}, State=#state{tid=TID}) - | |
| WorkerAddrs = [ libp2p_crypto:p2p_to_pubkey_bin(W#worker.target) || W <- State#state.workers, W#worker.target /= undefined, W#worker.kind /= seed ], | ||
| try libp2p_peerbook:random(Peerbook, [LocalAddr|WorkerAddrs]) of | ||
| {Addr, _} -> | ||
| lager:debug("found target ~p, assigning to worker ~p",[Addr, WorkerPid]), | ||
| [Addr]; | ||
| false -> | ||
| lager:debug("cannot get target as no peers or already connected to all peers",[]), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -213,6 +213,18 @@ handle_cast({request_target, Index, WorkerPid, _WorkerRef}, State=#state{tid=TID | |
| {keys, State#state.group_keys}]}}, | ||
| libp2p_group_worker:assign_target(WorkerPid, {Target, ClientSpec}), | ||
| {noreply, NewState}; | ||
| handle_cast({clear_target, _Kind, _WorkerPid, Ref}, State=#state{workers = Workers}) -> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but the target for each worker in the relcast server is always the same. maybe we should just drop the cast here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah you are right, relcast doesnt need this but maybe its better to handle the clear target cast as a noop instead, would avoid any future potential confusion if someone is in this flow and wondering why its not handled and doesnt have full context ?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, that's what I meant, sorry. I think a noop clause here with a good comment is the right thing. |
||
| lager:debug("clearing target for worker ~p ", [_WorkerPid]), | ||
| %% the ref is stable across restarts, so use that as the lookup key | ||
| case lookup_worker(Ref, #worker.ref, State) of | ||
| Worker=#worker{} -> | ||
| %% TODO - should the pid be set to undefined along with target ? | ||
| NewWorkers = lists:keyreplace(Ref, #worker.ref, Workers, | ||
| Worker#worker{target=undefined}), | ||
| State#state{workers=NewWorkers}; | ||
| _ -> | ||
| State | ||
| end; | ||
| handle_cast({handle_input, _Msg}, State=#state{close_state=closing}) -> | ||
| {noreply, State}; | ||
| handle_cast({handle_input, _Msg}, State=#state{store=Bad}) when Bad == not_started orelse | ||
|
|
||
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.
In theory the workerpid in here should match the one passed in the message?
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.
yes, Worker.pid() should equal WorkerPid from the message. As per the comment, the ref is used for the lookup as that is consistent across restarts of the worker