[PAN-2588] Create P2PNetwork Builder #1343
Conversation
mbaxter
force-pushed
the
PAN-2588/p2p-network-builder
branch
from
cc7baab
to
049ed1c
Compare
| * | ||
| * @return A stream of discovered peers on the network. | ||
| */ | ||
| Stream<DiscoveryPeer> getDiscoveredPeers(); |
There was a problem hiding this comment.
I'm not sure a Stream should be returned as a property. Streams are single use and have transient state, and mutations are not propagated back into the returning object. Could this be renamed streamDiscoveredPeers? This may need another PR to push this getFoo -> streamFoo done properly, but the name would then reflect reality.
Another option is to return the backing collection, but that looks to be an even deeper refactor to do properly.
There was a problem hiding this comment.
Up to now, we haven't used any special naming for streams. But we can discuss offline and maybe create a follow-up PR.
| @@ -70,7 +71,7 @@ | |||
| private static final long PEER_REFRESH_INTERVAL_MS = MILLISECONDS.convert(30, TimeUnit.MINUTES); | |||
|
|
|||
| protected final List<DiscoveryPeer> bootstrapPeers; | |||
| private final PeerRequirement peerRequirement; | |||
| private final List<PeerRequirement> peerRequirements = new CopyOnWriteArrayList<>(); | |||
There was a problem hiding this comment.
😄 CopyOnWriteArray is very concurrent friendly.
| peerBlacklist, | ||
| nodeWhitelistController, | ||
| nodePermissioningController, | ||
| metricsSystem); | ||
| checkArgument(vertx != null, "vertx instance cannot be null"); | ||
| this.vertx = vertx; | ||
|
|
||
| metricsSystem.createIntegerGauge( |
There was a problem hiding this comment.
Just to clarify, is the VertxPeerDiscoveryAgent created once and only once per server? If not the createIntegerGauge call will result in errors on the second and subsequent calls. Not sure there is any way to hedge this.
There was a problem hiding this comment.
Yes, currently only created once.
| @@ -741,4 +716,140 @@ private void onConnectionEstablished(final PeerConnection connection) { | |||
| connections.registerConnection(connection); | |||
| connectCallbacks.forEach(callback -> callback.accept(connection)); | |||
| } | |||
|
|
|||
| public static class Builder { | |||
There was a problem hiding this comment.
Not for this PR, but we should consider using a Builder library for these in the future to cut down on busy-work code. Lombok and Guava autoValues both have support for the builder pattern like we use it. Guava has the advantage we use part of it in our code already.
PR description
Create a
P2PNetworkbuilder. Using this builder pattern simplifies tests and should make it simpler to update theP2PNetworkin the future.This PR also:
networkpackagenettypackage tonetwork.nettyNettyP2PNetworkTestinto two tests: one that depends on the concrete implementation details of the network and one that doesn'tNettyP2PNetworktoDefaultP2PNeworkPeerRequirement's on thePeerDiscoveryAgentinstance rather than passing the requirements to the constructor - this allows us to construct the discovery agent outside of theP2PNetworkand pass it in as a dependencyReviewer's Notes
To review, it may be easier to look through individual commits in order to view changes without the noise of the file reorganization.