Reduce unhandled NetworkEvents #982
Merged
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
|
Reviewpad Report
|
joshuef
force-pushed
the
CmdLogging
branch
3 times, most recently
from
48e6346
to
a1812e5
Compare
maqi
reviewed
Nov 20, 2023
| channel: MsgResponder::FromSelf(sender), | ||
| }); | ||
| } else { | ||
| trace!("Replicate cmd to self received, ignoring"); |
There was a problem hiding this comment.
the log is bit confusing.
As I understand, it is a non-query request to self, which can be ignored, maybe due to replicate ?
There was a problem hiding this comment.
ah yeh, we'd be asking ourself to replicate data which we're already holding? So can be ignored
sn_networking/src/driver.rs
Outdated
| @@ -394,6 +394,8 @@ impl NetworkBuilder { | |||
|
|
|||
| // Gossipsub behaviour | |||
| let gossipsub_config = libp2p::gossipsub::ConfigBuilder::default() | |||
| // disable sending to ALL_PEERS subscribed to a topic, which is the default behaviour | |||
| .flood_publish(false) | |||
There was a problem hiding this comment.
this may cause a subscriber miss a msg?
it it is not known by ALL publisher ?
with the flood disabled, will forwarding` still get undertaken?
sn_networking/src/event.rs
Outdated
| // if the request is replication, we can handle it and send the OK response here, | ||
| // as we send that regardless of how we handle the request as its unimportant to the sender. | ||
| match request { | ||
| Request::Cmd(cmd) => { |
There was a problem hiding this comment.
sounds like the Replicate is the only Request::Cmd to be received across nodes?
I'd prefer narrow down then enum to be Cmd::Replicate only, to avoid later on it mess-up with other new work flow ?
This should avoid issues if that enum grows
maqi
approved these changes
Nov 20, 2023
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.
Description
Summary generated by Reviewpad on 20 Nov 23 10:37 UTC
This pull request includes the following changes:
In
sn_networking/src/cmd.rs:Querytype, it sends aNetworkEvent::QueryRequestReceived. Otherwise, it logs a message stating that a replicate command to self was received and it is ignored.In
sn_networking/src/driver.rs:flood_publish(false)).In
sn_networking/src/event.rs:RequestReceivedtoCmdRequestReceivedand added a new variantQueryRequestReceivedin theNetworkEventenum.In
sn_networking/src/swarm_driver.rs:Cmdtype, it sends a replicate OK response to the peer and handles the command separately. If the request is aQuerytype, it sends aNetworkEvent::QueryRequestReceived.In
sn_node/src/node.rs:handle_responsefunction. If the response is a replicate OK response, it logs a warning as it should have been handled earlier.handle_queryandhandle_node_cmdfunctions. Instead of returning a response, it now directly sends the response using thesend_responsefunction.In
sn_transfers/src/cashnotes/signed_spend.rs:custom_debugattribute to theSpendstruct.These changes improve the handling of requests and responses in the code.