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.

Sign up for GitHub

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

Jump to bottom

removing host.Peerstore().ClearAddrs from libp2pNode #1204

Merged
merged 2 commits into from
Aug 26, 2021
Merged

removing host.Peerstore().ClearAddrs from libp2pNode #1204

merged 2 commits into from
Aug 26, 2021

Conversation

vishalchangrani
Copy link
Contributor

  • adding a test to check no backoffs are added for 1-1 communication

@vishalchangrani
removing host.Peerstore().ClearAddrs from libp2pNode; adding a test t…
45dd21a 
…o check no backoffs are added for 1-1 communication
@vishalchangrani vishalchangrani self-assigned this Aug 25, 2021
vishalchangrani
vishalchangrani commented Aug 25, 2021
View reviewed changes
network/p2p/libp2pNode.go
@@ -359,10 +362,11 @@ func (n *Node) tryCreateNewStream(ctx context.Context, identity flow.Identity, m
default:
}

// remove the peer from the peer store if present
n.host.Peerstore().ClearAddrs(peerID)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

confirmed not needed

huitseeker
huitseeker approved these changes Aug 25, 2021
View reviewed changes
Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

As discussed in #1184 and

flow-go/network/p2p/libp2pNode.go

Lines 384 to 386 in 31c46bf

// TODO: why were we doing this? Is it okay to remove?
// remove the peer from the peer store if present
// n.host.Peerstore().ClearAddrs(peerID)

@vishalchangrani
Copy link
Contributor Author

bors merge

@bors
Copy link
Contributor

bors bot commented Aug 25, 2021

👎 Rejected by too few approved reviews

@codecov-commenter
Copy link

codecov-commenter commented Aug 26, 2021 •
edited

Codecov Report

Merging #1204 (ec7ac8a) into master (814b004) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1204   +/-   ##
=======================================
  Coverage   55.71%   55.72%           
=======================================
  Files         482      482           
  Lines       29644    29643    -1     
=======================================
+ Hits        16517    16519    +2     
+ Misses      10864    10862    -2     
+ Partials     2263     2262    -1
Flag Coverage Δ
unittests 55.72% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
network/p2p/libp2pNode.go 62.79% <100.00%> (-0.13%) ⬇️
engine/common/synchronization/engine.go 67.20% <0.00%> (ø)
cmd/util/ledger/migrations/storage_v4.go 42.16% <0.00%> (+0.60%) ⬆️
module/mempool/stdmap/identifier_map.go 84.37% <0.00%> (+3.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 814b004...ec7ac8a. Read the comment docs.

@synzhu
Copy link
Contributor

synzhu commented Aug 26, 2021

bors merge

bors bot added a commit that referenced this pull request Aug 26, 2021
@bors @vishalchangrani
Merge #1204
1337355 
1204: removing host.Peerstore().ClearAddrs from libp2pNode r=smnzhu a=vishalchangrani

- adding a test to check no backoffs are added for 1-1 communication

Co-authored-by: vishal <1117327+vishalchangrani@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Aug 26, 2021

This PR was included in a batch that successfully built, but then failed to merge into master (it was a non-fast-forward update). It will be automatically retried.

@bors
Copy link
Contributor

bors bot commented Aug 26, 2021

Build succeeded:

@bors bors bot merged commit f859a19 into master Aug 26, 2021
@bors bors bot deleted the vishal/remove_clearaddr branch August 26, 2021 00:58
@huitseeker huitseeker mentioned this pull request Aug 27, 2021
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@huitseeker huitseeker huitseeker approved these changes

@synzhu synzhu synzhu approved these changes

@yhassanzadeh13 yhassanzadeh13 yhassanzadeh13 approved these changes

Assignees

@vishalchangrani vishalchangrani

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@vishalchangrani @codecov-commenter @synzhu @huitseeker @yhassanzadeh13