This repository has been archived by the owner on Sep 26, 2019. It is now read-only.
[PAN-2731] Extract connection management from P2PNetwork #1538
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AbdelStark
reviewed
Jun 11, 2019
ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/api/ConnectCallback.java
Show resolved
Hide resolved
ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/api/MessageCallback.java
Outdated
Show resolved
Hide resolved
ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java
Outdated
Show resolved
Hide resolved
ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java
Show resolved
Hide resolved
ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java
Show resolved
Hide resolved
| .forEach(c -> c.disconnect(DisconnectReason.TOO_MANY_PEERS)); | ||
| } | ||
|
|
||
| private int prioritizeConnections(final RlpxConnection a, final RlpxConnection b) { |
There was a problem hiding this comment.
(Optional) Why not using first and second instead of a and b ? In general one letter variable could be confusing.
| * @return A negative value if {@code a} should be kept, a positive value is {@code b} should be | ||
| * kept | ||
| */ | ||
| private int compareDuplicateConnections(final RlpxConnection a, final RlpxConnection b) { |
There was a problem hiding this comment.
(Optional) Why not using first and second instead of a and b ? In general one letter variable could be confusing.
| if (!(o instanceof RemotelyInitiatedRlpxConnection)) { | ||
| return false; | ||
| } | ||
| final RemotelyInitiatedRlpxConnection that = (RemotelyInitiatedRlpxConnection) o; |
There was a problem hiding this comment.
(Optional) I would suggest not to use an intermediate local variable.
| @VisibleForTesting | ||
| int connectionCount() { | ||
| return pendingConnections.size() + rlpxAgent.getConnectionCount(); | ||
| private void attemptPeerConnections() { |
There was a problem hiding this comment.
🎉 Clear and concise method.
ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/rlpx/RlpxAgent.java
Outdated
Show resolved
Hide resolved
AbdelStark
approved these changes
Jun 11, 2019
Merged
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR description
This PR extracts connection management logic out of
DefaultP2PNetworkand into a set of new, more narrowly focused classes.RlpxAgentmanages the set of active and pending connections and enforces connection-related permissions and max peer limits.NettyConnectionInitializernow contains the netty-specific code for establishing connections. TheDefaultP2PNetworkis left to delegate actions to the more targeted classes it is composed of and do some coordination between these classes.Other changes include: