Skip to content
This repository has been archived by the owner on Feb 1, 2023. It is now read-only.

Refactor: simplify session peer management #275

Merged
merged 6 commits into from Mar 6, 2020
Merged

Conversation

dirkmc
Copy link
Contributor

@dirkmc dirkmc commented Mar 3, 2020

This refactor simplifies session peer management by

  • removing optimized / unoptimized peers
  • removing latency tracking from peer management (we now track latency in other places)
  • moving everything in session inside the run loop so there's no need for locking
  • moving responsibility for adding peers to the session when receiving a block / have into sessionWantSender
  • removing peerAvailabilityManager (now we just use the SessionPeerManager directly)

@dirkmc
Copy link
Contributor Author

dirkmc commented Mar 3, 2020

@Stebalien I still need to fix up the tests, just want to get your feedback on whether this approach makes sense
Tests are now fixed

@dirkmc dirkmc requested a review from Stebalien March 3, 2020 22:50
@@ -164,44 +171,25 @@ func (s *Session) ID() uint64 {

// ReceiveFrom receives incoming blocks from the given peer.
func (s *Session) ReceiveFrom(from peer.ID, ks []cid.Cid, haves []cid.Cid, dontHaves []cid.Cid) {
// The SessionManager tells each Session about all keys that it may be
// interested in. Here the Session filters the keys to the ones that this
// particular Session is interested in.
interestedRes := s.sim.FilterSessionInterested(s.id, ks, haves, dontHaves)
ks = interestedRes[0]
haves = interestedRes[1]
dontHaves = interestedRes[2]
// s.logReceiveFrom(from, ks, haves, dontHaves)

Copy link
Contributor Author

@dirkmc dirkmc Mar 3, 2020

Choose a reason for hiding this comment

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

I moved the isNewPeer check below to be inside sessionPeerSender as it simplifies the logic there and consolidates changes to session peers in one place

// Update want potential
s.sws.Update(from, ks, haves, dontHaves, isNewPeer)
// Inform the session want sender that a message has been received
s.sws.Update(from, ks, haves, dontHaves)

if len(ks) == 0 {
return
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the chunk of code below to be inside the run loop so we can get rid of locking


s.wm.BroadcastWantHaves(ctx, s.id, []cid.Cid{randomWant})

s.periodicSearchTimer.Reset(s.periodicSearchDelay.NextWaitTime())
}

// findMorePeers attempts to find more peers for a session by searching for
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I brought this function from inside of the SessionPeerManager to be in the Session instead, as it makes it easier to follow whats going on and simplifies the dependencies between Session, sessionWantSender and SessionPeerManager

@@ -12,7 +11,6 @@ import (
// sessionWants keeps track of which cids are waiting to be sent out, and which
// peers are "live" - ie, we've sent a request but haven't received a block yet
type sessionWants struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changes in sessionWants is basically just removing a bunch of locks that we no longer need as everything is inside the Session run loop now

@dirkmc dirkmc marked this pull request as ready for review March 4, 2020 16:09
@Stebalien Stebalien merged commit 981dfb1 into master Mar 6, 2020
@Stebalien Stebalien deleted the refactor/session-peers branch March 6, 2020 03:11
Jorropo pushed a commit to Jorropo/go-libipfs that referenced this pull request Jan 26, 2023
Refactor: simplify session peer management

This commit was moved from ipfs/go-bitswap@981dfb1
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants