[Networking] Refactoring Networking Layer for Improved Structure and Maintainability AND Eliminating Redundant Middleware Component #4664
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
yhassanzadeh13
requested review from
ramtinms,
AlexHentschel,
durkmurder and
jordanschalm
as code owners
FVM Benchstat comparisonThis branch with compared with the base branch onflow:master commit 104ca4f The command Collapsed results for better readability
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Co-authored-by: Misha <15269764+gomisha@users.noreply.github.com>
yhassanzadeh13
removed request for
ramtinms,
durkmurder,
jordanschalm and
AlexHentschel
…e-part-2' into yahya/6851-refactoring-middleware-part-2
kc1116
reviewed
Sep 6, 2023
cmd/node_builder.go
Outdated
| Middleware network.Middleware | ||
| Network network.Network | ||
| EngineRegistry network.EngineRegistry | ||
| UnderlayNetwork network.Underlay |
There was a problem hiding this comment.
Suggested change
| UnderlayNetwork network.Underlay | |
| NetworkUnderlay network.Underlay |
I think its fine to call struct fields with the Network* prefix, UnderlayNetwork sounds like it is an actual network rather than a component in a network. Lets use NetworkUnderlay in these instances.
kc1116
reviewed
Sep 6, 2023
network/p2p/libp2pNode.go
Outdated
| AddPeer(ctx context.Context, peerInfo peer.AddrInfo) error | ||
| // ConnectToPeerAddrInfo connects to the peer with the given peer address information. | ||
| // This method is used to connect to a peer that is not in the peer store. | ||
| ConnectToPeerAddrInfo(ctx context.Context, peerInfo peer.AddrInfo) error |
There was a problem hiding this comment.
Suggested change
| ConnectToPeerAddrInfo(ctx context.Context, peerInfo peer.AddrInfo) error | |
| ConnectToPeer(ctx context.Context, peerInfo peer.AddrInfo) error |
kc1116
reviewed
Sep 6, 2023
| mw network.Middleware | ||
| mu sync.RWMutex | ||
| engines map[channels.Channel]network.MessageProcessor | ||
| middleware network.Underlay // the Underlay interface of the network layer |
There was a problem hiding this comment.
Suggested change
| middleware network.Underlay // the Underlay interface of the network layer | |
| networkUnderlay network.Underlay // the Underlay interface of the network layer |
Do we still need to use "middleware" at all?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Background:
This PR aims to address the issues raised in GH issue https://github.com/dapperlabs/flow-go/issues/6851, which highlights inefficiencies in the Flow blockchain's networking layer. The current three-layered architecture has led to complexities, making the system difficult to maintain and debug.
Changes:
MiddlewaretoNetworkcomponent.Specifically, the
Middleware.goimplementation has been removed. Instead, its functionality has been revamped and refactored into an interface integrated directly within theNetwork.go. Now, ourNetwork.gofile embodies three distinct interfaces—Network,Adapter, andMiddleware—each serving different components.Network Interface: Provides functionalities exposed to the Flow protocol layer (i.e., engines). This includes registering channels and services like
BlobServiceandPingService.Adapter Interface: Serves as the networking layer for individual conduits, allowing them to send various types of messages, such as unicast, multicast, and publish.
Middleware Interface: Offers functionalities to lower-level networking components like libp2p, including channel subscriptions and updating addresses for authorized participants.
We acknowledge that the naming conventions for these interfaces may need to be revised. While we welcome suggestions for better names, we have retained the current names to facilitate a more streamlined PR review process for this first round.
Your feedback on the naming or the structural changes themselves would be greatly appreciated.