[NC-2056] Remove vertx from discovery tests #539
Conversation
Remove circular reference between agent and controller. Isolate networking code within the agent. Simplify observer handling.
Move signing logic into DiscoveryController.
Instantiate controller only when we have our advertisedPeer determined, so that the controller is initialized with the data it needs.
Remove vertx from discovery tests. Add helpers to mock message sending between discovery agents.
| .listen( | ||
| config.getBindPort(), | ||
| config.getBindHost(), | ||
| res -> { |
There was a problem hiding this comment.
Per style guild lambdas should be 1-3 lines.
| @@ -105,7 +103,9 @@ | |||
|
|
|||
| private final AtomicBoolean started = new AtomicBoolean(false); | |||
|
|
|||
| private final PeerDiscoveryAgent agent; | |||
| private final SECP256K1.KeyPair keypair; | |||
| final DiscoveryPeer self; | |||
There was a problem hiding this comment.
Not a fan of self outside python (nor of this inside of python). Could this become discoveryPeer or localPeer or something else?
My concern is that it maps to the same concept and so I keep reading it as this.
| this.nextAvailablePort = nextAvailablePort; | ||
| } | ||
|
|
||
| public AgentBuilder setBootstrapPeers(final List<DiscoveryPeer> peers) { |
There was a problem hiding this comment.
When I restored idiomatic getters/setters I didn't do setters for Builders (stuff that returned this). For consistency could you change it to just ?
| when(agent.getAdvertisedPeer()).thenReturn(peers[0]); | ||
| final MockPeerDiscoveryAgent agent = mock(MockPeerDiscoveryAgent.class); | ||
| when(agent.getAdvertisedPeer()).thenReturn(peers.get(0)); | ||
| DiscoveryPeer self = peers.get(0); |
There was a problem hiding this comment.
Again, self as a variable may cause python confusion with this, an alternate name would improve readability.
| private PeerDiscoveryController controller; | ||
| private DiscoveryPeer peer; | ||
| private DiscoveryPeer self; |
There was a problem hiding this comment.
Again, self may not be the best name here. Perhaps thisPeer?
| @Override | ||
| public long setPeriodic(final long delay, final TimerHandler handler) { | ||
| long id = nextId.incrementAndGet(); | ||
| periodicHandlers.put(id, handler); |
There was a problem hiding this comment.
Do we have any test cases that depend on different delays between peers for timers or handlers?
There was a problem hiding this comment.
We're not doing anything very complicated with the timers. We can always extend this in the future if we need to.
| public void runTimerHandlers() { | ||
| List<TimerHandler> handlers = new ArrayList<>(); | ||
| timerHandlers.forEach((id, handler) -> handlers.add(handler)); | ||
| timerHandlers.clear(); |
There was a problem hiding this comment.
handlers = timerHandlers.values(); would be better.
There was a problem hiding this comment.
The docs say values() is a view into the collection. I'm making a copy so that timers added as we run the handlers aren't run as part of this round of processing. Will add a comment.
| } | ||
|
|
||
| public void runPeriodicHandlers() { | ||
| List<TimerHandler> handlers = new ArrayList<>(); |
There was a problem hiding this comment.
Could be a 1 liner - periodicHandlers.forEach((id, handler) -> handler.handle); or periodicHandlers.values().forEach(TimerHandler::handler);
Unless you are seeing concurrent modification in which case it would be a 2 liner storing values in a List. If you are seeing concurrent modification it would be worth a comment.
| * affect the state machine of the Peer, and the table is only concerned with storing peers, | ||
| * keeping them alive and tracking their state. It is not bothered to service requests. | ||
| * </ol> | ||
| * messages via UDP. |
There was a problem hiding this comment.
/**
* The peer discovery agent is the network component that sends and receives messages peer discovery
* messages via UDP.
*/
^This is strangely worded and not really informative.
| this.config = config; | ||
| this.keyPair = keyPair; | ||
| this.peerTable = new PeerTable(keyPair.getPublicKey().getEncodedBytes(), 16); | ||
| this.controller = |
There was a problem hiding this comment.
interesting, did you remove that circular dependency thing?
|
|
||
| public abstract CompletableFuture<?> stop(); | ||
|
|
||
| public CompletableFuture<?> start() { |
There was a problem hiding this comment.
wow- interesting, so this only uses netty now?
There was a problem hiding this comment.
No, but I moved all the vertx code into a subclass. Shouldn't be too much work to migrate to netty now.
| new DiscoveryPeer( | ||
| id, | ||
| config.getAdvertisedHost(), | ||
| localAddress.getPort(), |
There was a problem hiding this comment.
why do we have two of localAddress.getPort() here? is it related to:
final String host = config.getBindHost();
final int port = config.getBindPort();
There was a problem hiding this comment.
That's how it behaved before. But it does seem wrong - I'll try to follow-up with a fix so I don't keep pushing stuff into this PR.
| String host = sourceEndpoint.getHost(); | ||
| int port = sourceEndpoint.getUdpPort(); | ||
| final DiscoveryPeer peer = new DiscoveryPeer(packet.getNodeId(), host, port, tcpPort); | ||
| controller.ifPresent(c -> c.onMessage(packet, peer)); |
There was a problem hiding this comment.
I kind of hate variables named c, but it's just a short lambda so, also w/e
There was a problem hiding this comment.
Yeah, its a super short lambda so its pretty obvious what c is. I'm a fan of quality variable names, but in cases like this I sometimes go for terseness.
| import static java.util.Collections.emptyList; | ||
| import static java.util.concurrent.TimeUnit.MILLISECONDS; | ||
| import static java.util.concurrent.TimeUnit.SECONDS; | ||
| import static tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PeerTable.AddResult.Outcome; | ||
|
|
||
| import tech.pegasys.pantheon.crypto.SECP256K1; |
There was a problem hiding this comment.
nice, this looks good. I don't think it'll be that bad to merge
| @@ -0,0 +1,39 @@ | |||
| /* | |||
There was a problem hiding this comment.
interesting idea, I like it
| @@ -296,7 +296,7 @@ public void rejectPeerWithNoSharedCaps() throws Exception { | |||
| new PeerBlacklist(), | |||
| new NoOpMetricsSystem(), | |||
| new NodeWhitelistController(PermissioningConfiguration.createDefault()))) { | |||
| final int listenPort = listener.getSelf().getPort(); | |||
| final int listenPort = listener.getLocalPeerInfo().getPort(); | |||
There was a problem hiding this comment.
local peer is a much more sane name, I like that
|
nice I think it looks good, I only really looked at the Agent, which is where most of the important changes are I think, and then at the Controller too, 'cause it's related to what I'm working on rn, the tests I didn't look at too closely |
PR description
Previously, our peer discovery tests used vertx to set up real sockets to communicate between discovery agents within the tests. This was a source of non-determinacy causing our tests to randomly fail. This PR reorganizes the discovery code in order to isolate vertx.
Changes include:
Fixed Issue(s)
NC-2056