This repository has been archived by the owner on Feb 1, 2023. It is now read-only.
Be less aggressive when pruning peers from session #276
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dirkmc
commented
Mar 5, 2020
| @@ -56,7 +56,7 @@ func (sw *sessionWants) GetNextWants(limit int) []cid.Cid { | |||
| func (sw *sessionWants) WantsSent(ks []cid.Cid) { | |||
| now := time.Now() | |||
| for _, c := range ks { | |||
| if _, ok := sw.liveWants[c]; !ok { | |||
| if _, ok := sw.liveWants[c]; !ok && sw.toFetch.Has(c) { | |||
There was a problem hiding this comment.
This was causing us to sometimes re-broadcast a want for which a block had already been received
Stebalien
reviewed
Mar 6, 2020
Stebalien
reviewed
Mar 6, 2020
| // If we already received a block for the want, ignore any HAVE for | ||
| // the want | ||
| if blkCids.Has(c) { | ||
| continue |
There was a problem hiding this comment.
Would this prevent us from adding this peer to the session?
There was a problem hiding this comment.
Adding the peer to the session happens before processUpdates() gets called. However this does interfere with consecutive DONT_HAVE counting so I refactored to avoid interference.
| // Before removing the peer from the session, check if the peer | ||
| // sent us a HAVE for a block that we want | ||
| peerHasWantedBlock := false | ||
| for c := range sws.wants { |
There was a problem hiding this comment.
This is in a goroutine. Is this safe?
dirkmc
force-pushed
the
fix/prune-dont-have
branch
from
bba02f3
to
f74c469
Compare
Stebalien
force-pushed
the
fix/prune-dont-have
branch
from
f74c469
to
22f0c79
Compare
|
(rebasing to get the tests working) |
|
Let me take a look at that failed test now... |
Stebalien
approved these changes
Mar 6, 2020
| @@ -340,7 +340,7 @@ func (s *Session) broadcastWantHaves(ctx context.Context, wants []cid.Cid) { | |||
| // Search for providers who have the first want in the list. | |||
| // Typically if the provider has the first block they will have | |||
| // the rest of the blocks also. | |||
| log.Warnf("Ses%d: FindMorePeers with want 0 of %d wants", s.id, len(wants)) | |||
| log.Warnf("Ses%d: FindMorePeers with want %s (1st of %d wants)", s.id, lu.C(wants[0]), len(wants)) | |||
There was a problem hiding this comment.
OT note: this should be an info at best.
Jorropo
pushed a commit
to Jorropo/go-libipfs
that referenced
this pull request
Jan 26, 2023
Be less aggressive when pruning peers from session This commit was moved from ipfs/go-bitswap@418d88c
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently we prune peers from the session when they send us several DONT_HAVEs in a row.
However if the peer sent us a HAVE for a block that we want, we should not prune the peer.
Depends on #275