Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Source messenger on ProcessReceivedMessage #5365

Merged
merged 8 commits into from Jun 26, 2023

Conversation

sstanculeanu
Copy link
Contributor

@sstanculeanu sstanculeanu commented Jun 21, 2023

Reasoning behind the pull request

  • implement a proper way to respond on the messenger that initiated the request

Proposed changes

  • updated mx-chain-communication-go to latest which has source messenger as parameter on ProcessReceivedMessage

Testing procedure

  • will be tested with feat branch

Pre-requisites

Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:

  • was the PR targeted to the correct branch?
  • if this is a larger feature that probably needs more than one PR, is there a feat branch created?
  • if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

Base automatically changed from requesters_resolvers_full_archive_network to feat/multiple_p2p_messengers June 21, 2023 11:36
@sstanculeanu sstanculeanu marked this pull request as ready for review June 21, 2023 11:36
@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Patch coverage: 98.95% and project coverage change: -0.01 ⚠️

Comparison is base (9d48c22) 80.33% compared to head (ef527c5) 80.32%.

❗ Current head ef527c5 differs from pull request most recent head 177c5ff. Consider uploading reports for the commit 177c5ff to get more accurate results

Additional details and impacted files
@@                       Coverage Diff                        @@
##           feat/multiple_p2p_messengers    #5365      +/-   ##
================================================================
- Coverage                         80.33%   80.32%   -0.01%     
================================================================
  Files                               695      695              
  Lines                             90497    90486      -11     
================================================================
- Hits                              72698    72685      -13     
- Misses                            12630    12631       +1     
- Partials                           5169     5170       +1     
Impacted Files Coverage Δ
...uestersContainer/baseRequestersContainerFactory.go 80.95% <ø> (ø)
...esolverscontainer/baseResolversContainerFactory.go 82.75% <ø> (ø)
epochStart/bootstrap/process.go 84.40% <ø> (ø)
heartbeat/sender/multikeyHeartbeatSender.go 86.40% <ø> (-0.11%) ⬇️
factory/network/networkComponents.go 88.02% <93.75%> (-0.08%) ⬇️
consensus/spos/worker.go 82.14% <100.00%> (ø)
...uestersContainer/baseRequestersContainerFactory.go 75.29% <100.00%> (ø)
dataRetriever/resolvers/headerResolver.go 100.00% <100.00%> (ø)
dataRetriever/resolvers/miniblockResolver.go 100.00% <100.00%> (ø)
...aRetriever/resolvers/peerAuthenticationResolver.go 100.00% <100.00%> (ø)
... and 11 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@sstanculeanu sstanculeanu changed the title Network field on p2p message Source messenger on ProcessReceivedMessage Jun 22, 2023
@iulianpascalau iulianpascalau self-requested a review June 23, 2023 07:13
@@ -13,8 +13,8 @@ import (
type FactoryArgs struct {
RequesterConfig config.RequesterConfig
ShardCoordinator sharding.Coordinator
MainMessenger dataRetriever.TopicMessageHandler
FullArchiveMessenger dataRetriever.TopicMessageHandler
MainMessenger p2p.Messenger
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should evaluate that we can get rid of providing the messenger on the resolvers as the messenger is now provided on each data request. (next PR)

dataRetriever/topicSender/topicResolverSender.go Outdated Show resolved Hide resolved
heartbeat/sender/multikeyHeartbeatSender.go Outdated Show resolved Hide resolved
p2p/constants.go Outdated
type MessageHandlerType = p2p.MessageHandlerType

// RegularMessageHandler defines a message handler for the main network
const RegularMessageHandler = p2p.RegularMessageHandler
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

iulianpascalau
iulianpascalau previously approved these changes Jun 23, 2023
@bogdan-rosianu bogdan-rosianu self-requested a review June 26, 2023 08:32
}
err = fullArchiveMessenger.SetPeerShardResolver(peerShardMapperFullArch)
if err != nil {
log.Error("error setting NewPeerShardMapper in p2p messenger for full archive network", "error", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

log.Error("error setting a peer shard mapper in p2p messenger for full archive network", "error", err)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pushed

@sstanculeanu sstanculeanu merged commit dacd359 into feat/multiple_p2p_messengers Jun 26, 2023
4 checks passed
@sstanculeanu sstanculeanu deleted the network_on_p2p_message branch June 26, 2023 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants