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

Fix direct connections processor #4390

Merged
merged 5 commits into from
Aug 24, 2022

Conversation

iulianpascalau
Copy link
Contributor

Description of the reasoning behind the pull request (what feature was missing / how the problem was manifesting itself / what was the motive behind the refactoring)

  • Fixed the direct connection processor to not continuously send messages to the connected peers

Proposed Changes

  • clean the map only after notifying the new peers and then insert all connected peers so in case of no connected peers changes we won't resend the messages.

Testing procedure

  • standard testing procedure should reveal problems (continuously resend) if the test is carried with the log level *:DEBUG,heartbeat/processor:TRACE. If ran with this branch the sending will be done only on new connected peers.

@codecov-commenter
Copy link

codecov-commenter commented Aug 22, 2022

Codecov Report

Merging #4390 (2a4491d) into rc/2022-july (e15520a) will increase coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head 2a4491d differs from pull request most recent head 6fb11aa. Consider uploading reports for the commit 6fb11aa to get more accurate results

@@               Coverage Diff                @@
##           rc/2022-july    #4390      +/-   ##
================================================
+ Coverage         73.78%   73.82%   +0.03%     
================================================
  Files               675      675              
  Lines             86837    86853      +16     
================================================
+ Hits              64072    64116      +44     
+ Misses            17968    17939      -29     
- Partials           4797     4798       +1     
Impacted Files Coverage Δ
factory/heartbeatV2Components.go 74.60% <100.00%> (+0.20%) ⬆️
heartbeat/processor/directConnectionsProcessor.go 100.00% <100.00%> (ø)
storage/txcache/txListBySenderMap.go 97.50% <0.00%> (-2.50%) ⬇️
p2p/libp2p/netMessenger.go 80.02% <0.00%> (+3.91%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@iulianpascalau iulianpascalau marked this pull request as draft August 22, 2022 14:26
@iulianpascalau iulianpascalau changed the title Fix direct connections processor [WIP]Fix direct connections processor Aug 22, 2022
@iulianpascalau iulianpascalau changed the title [WIP]Fix direct connections processor Fix direct connections processor Aug 22, 2022
@iulianpascalau iulianpascalau marked this pull request as ready for review August 22, 2022 16:08
sstanculeanu
sstanculeanu previously approved these changes Aug 22, 2022
@AdoAdoAdo AdoAdoAdo self-requested a review August 23, 2022 06:09
@@ -92,9 +98,22 @@ func (dcp *directConnectionsProcessor) startProcessLoop(ctx context.Context) {
}

func (dcp *directConnectionsProcessor) sendMessageToNewConnections() {
if dcp.isCurrentNodeValidator() {
log.Debug("directConnectionsProcessor.sendMessageToNewConnections current node is validator, will not send send messages to connected peers")
Copy link
Contributor

Choose a reason for hiding this comment

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

double send in the message.
also is this method called for each connection? I think there will be too many logs for this.

Copy link
Contributor Author

@iulianpascalau iulianpascalau Aug 23, 2022

Choose a reason for hiding this comment

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

No, it will be logged once every 1 minute, as it is configured now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed message

Copy link
Contributor

@gabi-vuls gabi-vuls left a comment

Choose a reason for hiding this comment

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

System test passed.

@gabi-vuls gabi-vuls merged commit 5357e61 into rc/2022-july Aug 24, 2022
@gabi-vuls gabi-vuls deleted the fix-direct-connections-processor branch August 24, 2022 08:29
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.

5 participants