This repository has been archived by the owner on Feb 1, 2023. It is now read-only.
Bitswap Refactor #3: Extract sessions to package #30
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
ghost
assigned 
hannahhoward
changed the title
hannahhoward
force-pushed
the
feat/extract-managers
branch
from
c320f0a
to
03f4615
Compare
hannahhoward
force-pushed
the
feat/extract-sessions
branch
from
5eb506e
to
604ae5e
Compare
hannahhoward
force-pushed
the
feat/extract-managers
branch
from
03f4615
to
ac45ed0
Compare
- moved sessions out of main bitswap package - modified session manager to manage all sessions - moved get functions to their own package so sessions can directly BREAKING CHANGE: SessionsForBlock, while not used outside of Bitswap, has been removed, and was an exported function
hannahhoward
force-pushed
the
feat/extract-sessions
branch
from
604ae5e
to
40aa1fb
Compare
|
@Stebalien @whyrusleeping @michaelavila @eingenito @lanzafame @schomatis @parkan This is now the next part to review, if/when you have time. |
Stebalien
approved these changes
Dec 12, 2018
session/session.go
Outdated
| logging "github.com/ipfs/go-log" | ||
| loggables "github.com/libp2p/go-libp2p-loggables" | ||
| peer "github.com/libp2p/go-libp2p-peer" | ||
| ) | ||
|
|
||
| const activeWantsLimit = 16 | ||
|
|
||
| // SessionWantmanager is an interface that can be used to request blocks |
| type SessionWantManager interface { | ||
| WantBlocks(ctx context.Context, ks []cid.Cid, peers []peer.ID, ses uint64) | ||
| CancelWants(ctx context.Context, ks []cid.Cid, peers []peer.ID, ses uint64) | ||
| } |
There was a problem hiding this comment.
Thought for a future direction (we can leave this as-is for now, I really like the architecture):
type WantManager interface {
AddWants(ctx context.Context, ks []cid.Cid, peers []peer.ID)
CancelWants(ctx context.Context, ks []cid.Cid, peers []peer.ID)
}That is, we're currently:
- Constructing a session, passing in a session ID and a SessionWantManager.
- Expecting that SessionWantManager to pass the ID back for each request.
Instead, we could construct a thin helper type implementing the WantManager interface that remembers the session ID.
But we can leave this for later, if ever.
sessionmanager/sessionmanager.go
Outdated
| sm.sessLk.Lock() | ||
| sm.sessions = append(sm.sessions, session) | ||
| sm.sessLk.Unlock() | ||
| go func() { | ||
| for { |
There was a problem hiding this comment.
oh, you're right. good catch. :)
remove for loop not needed, cleanup spelling
Stebalien
reviewed
Dec 13, 2018
| defer cancel() | ||
| select { | ||
| case <-sm.ctx.Done(): | ||
| sm.removeSession(session) |
There was a problem hiding this comment.
Nit: duplicate code. We can replace this with:
defer cancel()
select {
case <-sm.ctx.Done():
case <-ctx.Done():
}
sm.removeSession(session)
|
(this has been up for long enough, feel free to merge when you're ready) |
Jorropo
pushed a commit
to Jorropo/go-libipfs
that referenced
this pull request
Jan 26, 2023
Bitswap Refactor ipfs#3: Extract sessions to package This commit was moved from ipfs/go-bitswap@1e9b2c4
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.
Goals
Modularize Bitswap in preparation for attempts to optimize bitswap further
child of #26
Implementation
BREAKING CHANGE: SessionsForBlock, while not used outside of Bitswap, has been removed, and was an exported function