fix(sessions): explicitly connect found peers #56
Conversation
when providers are found in a session, explicitly connect them so they get added to the peer manager fix #53
ghost
assigned 
| @@ -222,7 +222,7 @@ func (s *Session) SetBaseTickDelay(baseTickDelay time.Duration) { | |||
| } | |||
| } | |||
|
|
|||
| const provSearchDelay = time.Second * 10 | |||
There was a problem hiding this comment.
This is pretty low. What was the reasoning behind this change?
There was a problem hiding this comment.
To match time time for regular GetBlocks in Bitswap w/o sessions... especially if we're going to merge these two.
There was a problem hiding this comment.
But transparently in this case, for testing. And it should be configurable.
There was a problem hiding this comment.
To match time time for regular GetBlocks in Bitswap w/o sessions... especially if we're going to merge these two.
The timer for GetBlocks is a bit different. That's an initial delay before finding a provider for a single block. In the past, we didn't even delay this. Subsequent FindProviders calls are controlled by the rebroadcastWorker and sent out every 10 seconds (for random blocks).
There was a problem hiding this comment.
yea in this case, after a block is received, it switches over to using the tick delay, which is 500ms + 3 * average latency per block. So, somewhat similar.
There was a problem hiding this comment.
provSearchDelay is only used at the beginning of a session for the first call to FindProvidersAsync, if no blocks come back from any peers.
There was a problem hiding this comment.
Ah, I see. Yeah, that makes total sense.
(thanks for digging into this)
| } | ||
| wg.Wait() |
There was a problem hiding this comment.
Nit: We don't really need to wait here.
There was a problem hiding this comment.
true I think... though we may need to once we dedup with providers worker. But agreed, maybe I'll remove wait group knowledge for now.
|
@Stebalien I went ahead and removed the waitgroup code just for now and made the provSearchDelay configurable (without changing the default value). I'd like to revisit the default value issue when I work on making regular GetBlocks just use sessions (see comments above), but for now, this should work. Lemme know if this is LGTM |
Remove sync.waitGroup in SessionPeerManager till it's needed
hannahhoward
force-pushed
the
feat/connect-providers-in-sessions
branch
from
ce2e171 to
6f7a77e
Compare
…s-in-sessions fix(sessions): explicitly connect found peers This commit was moved from ipfs/go-bitswap@fa9aec8
Goal
When providers are found in a session, explicitly connect them so they get added to the peer manager
fix #53
Implementation
For Discussion
n/a