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

Bitswap Refactor #1: Session Manager & Extract Want Manager #28

Merged
merged 1 commit into from
Dec 5, 2018

Conversation

hannahhoward
Copy link
Contributor

Goals

Modularize Bitswap in preparation for attempts to optimize bitswap further
child of #26

Implementation

  • Put WantManager in a sub-package with public interface and private data fields
  • Make a SessionManager to track sessions for a given bitswap instance (taking out session data fields from BS itself)
  • Move message queue interface, which lived inside of WantManager, into its own package, added unit test

@ghost ghost assigned hannahhoward Dec 4, 2018
@ghost ghost added the status/in-progress In progress label Dec 4, 2018
@hannahhoward hannahhoward changed the title Bitswap Refactor #1 Bitswap Refactor #1: Session Manager & Extract Want Manager Dec 4, 2018
@ghost ghost removed the status/in-progress In progress label Dec 4, 2018
@hannahhoward hannahhoward reopened this Dec 4, 2018
@ghost ghost added the status/in-progress In progress label Dec 4, 2018
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

The code looks good to me. The iterate method bugs me a bit but I'd be fine figuring that out later.


type IterateSessionFunc func(session exchange.Fetcher)

// SessionsForBlock returns a slice of all sessions that may be interested in the given cid
Copy link
Member

Choose a reason for hiding this comment

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

Wrong doc comment.

var out []*Session
for _, s := range bs.sessions {
bs.sm.IterateSessions(func(session exchange.Fetcher) {
s := session.(*Session)
Copy link
Member

Choose a reason for hiding this comment

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

Having this cast is a bit funky. I get that this is to avoid a dependency issue but I wonder if there's a better way. Would it make sense to move sessions into their own package? That way, SessionManager could operate over concrete sessions.

Unfortunately, I don't think that'll work as Session appears to need access to bitswap internals. An alternative is to create a Session interface that exposes an InterestedIn method.

I'd be fine punting these questions till later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey!

Let's punt till later since two PR's forward Sessions end up in their own package, and this cast is gone :)

@hannahhoward
Copy link
Contributor Author

IterateSessions disappears in a later PR so I wouldn't worry to much about it. I will fix the comment issue though.

Extract session manager from bitswap
Extract session manager & want manager to package
Move want manager message queue to seperate file
Move Message Queue to subpackage
Respond to PR Comments
@hannahhoward hannahhoward force-pushed the feat/extract-to-package branch from 4d5134c to 69d063b Compare December 4, 2018 19:46
@Stebalien
Copy link
Member

This is mostly just moving code around so I'm going to leave it at one LGTM.

@Stebalien Stebalien merged commit c9fee08 into master Dec 5, 2018
@ghost ghost removed the status/in-progress In progress label Dec 5, 2018
@parkan
Copy link

parkan commented Dec 10, 2018

continues in #29

@parkan parkan deleted the feat/extract-to-package branch December 10, 2018 21:24
Jorropo pushed a commit to Jorropo/go-libipfs that referenced this pull request Jan 26, 2023
Bitswap Refactor ipfs#1: Session Manager & Extract Want Manager

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

3 participants