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

Bitswap Refactor #2: Extract PeerManager From Want Manager + Unit Test #29

Merged
merged 5 commits into from
Dec 11, 2018

Conversation

hannahhoward
Copy link
Contributor

@hannahhoward hannahhoward commented Dec 4, 2018

Goals

Modularize Bitswap in preparation for attempts to optimize bitswap further

child of #26

Implementation

  • Seperated out a PeerManager, which was part of WantManager but doesn't seem at all tied to managing peers
  • Made unit tests for PeerManager and WantManager
  • Discovered issues while unit testing with synchronization of calls that lead to unpredictable output and refactored both classes to process messages serially to provide a predictable interface (apologies for bringing in erlang like actor pattern habits of using message passing)
  • Moved SendBlocks function which seemed like a one off that had little to do with WantManager back in main bitswap package for now
  • Comment remaining public interfaces

Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

LGTM. Interesting solution to getting deterministic results on the goroutine channel selection logic.

@hannahhoward hannahhoward changed the base branch from feat/extract-to-package to master December 5, 2018 17:25
NewMessageSender(context.Context, peer.ID) (bsnet.MessageSender, error)
}

// MessageQueue implements queuee of want messages to send to peers

Choose a reason for hiding this comment

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

typo: s/queuee/queue

@@ -30,7 +38,8 @@ type MessageQueue struct {
done chan struct{}
}

func New(p peer.ID, network bsnet.BitSwapNetwork) *MessageQueue {
// New creats a new MessageQueues

Choose a reason for hiding this comment

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

nitpick FYI: comments should end with a fullstop, see here, please don't feel it necessary to update all the comments but moving forward it would be great to standardize on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea I can commit to this one going forward. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

went ahead and implemented this across bitswap as best I could, but put this in a seperate commit (including spelling change above) to be merged right after, so as to avoid making review harder. #31

Copy link

Choose a reason for hiding this comment

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

@lanzafame is your review a ✔️ given changes in #31?

Copy link
Contributor

@michaelavila michaelavila left a comment

Choose a reason for hiding this comment

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

Looks good. I like the Scott Encoding to handle different message types on the same queue, kudos.

@hannahhoward
Copy link
Contributor Author

I'm gonna go head and merge after a quick rebase.

Seperates the functions of tracking wants from tracking peers
Unit tests for both peer manager and want manager
Refactor internals of both to address synchonization issues discovered
in tests
Finishing adding comments to WantManager and PeerManager, refactor message structure for type
safety, add sending messages test
@hannahhoward hannahhoward merged commit e9b80b6 into master Dec 11, 2018
@ghost ghost removed the status/in-progress In progress label Dec 11, 2018
Jorropo pushed a commit to Jorropo/go-libipfs that referenced this pull request Jan 26, 2023
Bitswap Refactor ipfs#2: Extract PeerManager From Want Manager + Unit Test

This commit was moved from ipfs/go-bitswap@e9b80b6
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.

6 participants