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

feat(kademlia): restrict peer quick pruning on net errors only #2599

Merged
merged 2 commits into from Oct 14, 2021

Conversation

janos
Copy link
Member

@janos janos commented Oct 14, 2021

This PR adds a very small addition to the quick pruning to prone only on network related errors. Thanks @acud for the idea.


This change is Reviewable

@janos janos added the ready for review The PR is ready to be reviewed label Oct 14, 2021
@janos janos requested review from acud and istae October 14, 2021 09:37
@janos janos self-assigned this Oct 14, 2021
@codecov-commenter
Copy link

codecov-commenter commented Oct 14, 2021

Codecov Report

Merging #2599 (b0dbf32) into master (e53b36c) will decrease coverage by 0.16%.
The diff coverage is 20.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2599      +/-   ##
==========================================
- Coverage   63.54%   63.38%   -0.17%     
==========================================
  Files         232      232              
  Lines       25937    25945       +8     
==========================================
- Hits        16482    16444      -38     
- Misses       7958     7996      +38     
- Partials     1497     1505       +8     
Impacted Files Coverage Δ
pkg/topology/kademlia/kademlia.go 74.53% <20.00%> (-1.55%) ⬇️
pkg/localstore/subscription_push.go 71.84% <0.00%> (-9.71%) ⬇️
pkg/p2p/libp2p/upnp.go 59.37% <0.00%> (-6.25%) ⬇️
pkg/localstore/subscription_pull.go 84.24% <0.00%> (-2.74%) ⬇️
pkg/p2p/libp2p/libp2p.go 59.68% <0.00%> (-1.95%) ⬇️
pkg/retrieval/retrieval.go 64.06% <0.00%> (-0.32%) ⬇️
cmd/bee/cmd/cmd.go 65.83% <0.00%> (-0.22%) ⬇️
cmd/bee/cmd/start.go 2.38% <0.00%> (+<0.01%) ⬆️

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 e53b36c...b0dbf32. Read the comment docs.

Copy link
Contributor

@mrekucci mrekucci left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @acud, @istae, and @janos)


pkg/topology/kademlia/kademlia.go, line 1506 at r1 (raw file):

// isNetworkError is checking various conditions that relate to network problems.
func isNetworkError(err error) bool {

Since the error might be wrapped I'd suggest using errors.As() function to check for those error types.

@sonarcloud
Copy link

sonarcloud bot commented Oct 14, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@ldeffenb
Copy link
Collaborator

Thanks for this, but it's still discouraging if you have a network connectivity issue when you have just started a bee node. You can watch the 100,000+ entry accumulated address book disappear within minutes!

Copy link
Contributor

@mrekucci mrekucci left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @acud and @istae)

Copy link
Member

@acud acud left a comment

Choose a reason for hiding this comment

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

Sweet :) thanks @janos 🙏

@janos janos merged commit faca395 into master Oct 14, 2021
@janos janos deleted the kademlia-net-err-prune branch October 14, 2021 14:05
@acud acud added this to the v1.3.0 milestone Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pull-request ready for review The PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants