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: measure "connect time" per-peer for peer scoring #431

Merged
merged 1 commit into from
Sep 21, 2023

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Sep 21, 2023

Prior to this, the "start" is the same for all peers, regardless of whether they come in from the indexer at different times; this makes it strictly local to the peer so we can score it accordingly.

I don't think this should make a material difference if we're getting the graphsync and http peers in an initial batch from the indexer. The current start-time calculation begins from before we even get candidates back, but they are all the same start. The actual value for this only matters for scoring candidates so mostly it's the relative value that matters. What we intended for this, however, was that if this number is 0 or close to it, it means we're already connected to the peer so we should try them first.

TransportProtocol is a public interface so this would technically be a breaking change.

prior to this, the "start" is the same for all peers, regardless of
whether they come in from the indexer at different times; this makes it
strictly local to the peer so we can score it accordingly
@codecov-commenter
Copy link

codecov-commenter commented Sep 21, 2023

Codecov Report

Merging #431 (4caba91) into main (a7c7002) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #431      +/-   ##
==========================================
- Coverage   76.65%   76.57%   -0.08%     
==========================================
  Files          85       85              
  Lines        6377     6377              
==========================================
- Hits         4888     4883       -5     
- Misses       1238     1241       +3     
- Partials      251      253       +2     
Files Changed Coverage Δ
pkg/retriever/graphsyncretriever.go 86.41% <100.00%> (+0.08%) ⬆️
pkg/retriever/httpretriever.go 86.82% <100.00%> (ø)
pkg/retriever/parallelpeerretriever.go 92.85% <100.00%> (-0.04%) ⬇️

... and 5 files with indirect coverage changes

@rvagg rvagg merged commit e976d19 into main Sep 21, 2023
9 checks passed
@rvagg rvagg deleted the rvagg/connect-time-adjustment branch September 21, 2023 10:00
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.

3 participants