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

[Merged by Bors] - sync: use randomized peers from the set of 20 peers with good latency #5263

Closed
wants to merge 2 commits into from

Conversation

dshulyak
Copy link
Contributor

@dshulyak dshulyak commented Nov 14, 2023

this change improves peer selection logic to split the load between 20 peers with good latency,
additionally it avoids suboptimal behavior when node will stick to the same peer when it would benefit switching to another:

  • peer had the best latency in the past, but temporarily faulty due to being overloaded
  • peer fails to negotiate protocols, but we didn't collect latency info to prioritize peers yet

in second case we should disconnect such peer, but our software registers protocol asynchronously
so we actually may drop honest peer, therefore we only workaround such problem.

@dshulyak dshulyak marked this pull request as ready for review November 14, 2023 08:19
Copy link

codecov bot commented Nov 14, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (8a0a6f2) 78.0% compared to head (4c8bdbb) 78.0%.

❗ Current head 4c8bdbb differs from pull request most recent head f20c310. Consider uploading reports for the commit f20c310 to get more accurate results

Files Patch % Lines
fetch/fetch.go 66.6% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           develop   #5263   +/-   ##
=======================================
  Coverage     78.0%   78.0%           
=======================================
  Files          266     266           
  Lines        31977   31985    +8     
=======================================
+ Hits         24964   24978   +14     
+ Misses        5499    5493    -6     
  Partials      1514    1514           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dshulyak dshulyak changed the title use randomized peers from the set of 20 peers with good latency sync: use randomized peers from the set of 20 peers with good latency Nov 14, 2023
@dshulyak
Copy link
Contributor Author

bors merge

spacemesh-bors bot pushed a commit that referenced this pull request Nov 14, 2023
…#5263)

this change improves peer selection logic to split the load between 20 peers with good latency,
additionally it avoids suboptimal behavior when node will stick to the same peer when it would benefit switching to another:
- peer had the best latency in the past, but temporarily faulty due to being overloaded
- peer fails to negotiate protocols, but we didn't collect latency info to prioritize peers yet

in second case we should disconnect such peer, but our software registers protocol asynchronously
so we actually may drop honest peer, therefore we only workaround such problem.
@spacemesh-bors
Copy link

Pull request successfully merged into develop.

Build succeeded:

@spacemesh-bors spacemesh-bors bot changed the title sync: use randomized peers from the set of 20 peers with good latency [Merged by Bors] - sync: use randomized peers from the set of 20 peers with good latency Nov 14, 2023
@spacemesh-bors spacemesh-bors bot closed this Nov 14, 2023
dshulyak added a commit that referenced this pull request Nov 15, 2023
…#5263)

this change improves peer selection logic to split the load between 20 peers with good latency,
additionally it avoids suboptimal behavior when node will stick to the same peer when it would benefit switching to another:
- peer had the best latency in the past, but temporarily faulty due to being overloaded
- peer fails to negotiate protocols, but we didn't collect latency info to prioritize peers yet

in second case we should disconnect such peer, but our software registers protocol asynchronously
so we actually may drop honest peer, therefore we only workaround such problem.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants