-
Notifications
You must be signed in to change notification settings - Fork 130
[PAN-2730] Create MaintainedPeers class #1484
[PAN-2730] Create MaintainedPeers class #1484
Conversation
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.
LGTM. Left few optional comments.
peer.getEnodeURL().isListening(), | ||
"Invalid enode url. Enode url must contain a non-zero listening port."); | ||
boolean wasAdded = maintainedPeers.add(peer); | ||
addedSubscribers.forEach(s -> s.onPeerAdded(peer, wasAdded)); |
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.
Why not invoking subscribers in parallel ? Since there is no particular check in the foreach loop it is doable.
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.
Leaving this as-is for now because I don't want to get into changing the Subscribers
interface.
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.
You don't have too.
I was just suggesting to do : addedSubscribers.parallelStream().forEach(s -> s.onPeerAdded(peer, wasAdded));
instead of addedSubscribers.forEach(s -> s.onPeerAdded(peer, wasAdded));
|
||
public boolean remove(final Peer peer) { | ||
boolean wasRemoved = maintainedPeers.remove(peer); | ||
removedCallbackSubscribers.forEach(s -> s.onPeerRemoved(peer, wasRemoved)); |
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.
Same as previous.
return maintainedPeers.stream(); | ||
} | ||
|
||
public interface PeerAddedCallback { |
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.
FunctionalInterface ?
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.
good call - done
PR description
Move peer maintenance list management into a new
MaintainedPeers
class. This change will help facilitate the break up of logic withinDefaultP2PNetwork
by decoupling the add / remove action from the handling of peers in this list.This change also makes tests more robust by exposing a public dependency that can be checked directly, rather than having to reach into the
DefaultP2PNetwork
to check a private field. Some connection-related checks were moved intoDefaultP2PNetwork.connect
in order to reduce the amount of custom logic executed when a new peer is added toMaintainedPeers
.