generated from ipfs/ipfs-repository-template
-
Notifications
You must be signed in to change notification settings - Fork 150
Closed
Labels
P1High: Likely tackled by core team if no one steps upHigh: Likely tackled by core team if no one steps updif/expertExtensive knowledge (implications, ramifications) requiredExtensive knowledge (implications, ramifications) requiredkind/enhancementA net-new feature or improvement to an existing featureA net-new feature or improvement to an existing feature
Description
Background
This is the ProviderQueryManager that is in an internal package of the bitswap client
| // ProviderQueryManager manages requests to find more providers for blocks | |
| // for bitswap sessions. It's main goals are to: | |
| // - rate limit requests -- don't have too many find provider calls running | |
| // simultaneously | |
| // - connect to found peers and filter them if it can't connect | |
| // - ensure two findprovider calls for the same block don't run concurrently | |
| // - manage timeouts | |
| type ProviderQueryManager struct { |
Notably it does almost nothing that's specific to Bitswap, or even the concept of data transfer sessions. Instead it is an opinionated content routing wrapper that does things like:
- Have max timeouts
- Have predetermined max provider records to use
- Does preconnects before returning the peers
- Deduplicates ongoing queries
- Rate limits requests
Unfortunately sometimes these opinions are too strong for consumers who'd like to experiment or use something else (e.g. higher rate limits, different timeouts, etc.
Proposal
- Move the ProviderQueryManager package into the routing package
- Refactor it to match the
FindProvidersAsyncmethod from go-libp2p's content routing interface - Continue using it by default in the bitswap client, but have an option to not use it
- Add some cleanup / options to the ProviderQueryManager for now and consider separating out the pieces later
Some advantages of this proposal are:
- If these wrappers are needed/useful elsewhere they can be
- While we can for now leave the ProviderQueryManager used by the Bitswap client by default we can add an option to disable it and just use the ContentRouting directly
- While doing 2 we can allow the user to easily provide a modified ProviderQueryManager if it's useful to them by just passing it in as the ContentRouter.
Alternatives Considered
- Do experiments in forks for the best configuration and upstream to everyone in bitswap/client -> Too cumbersome and inflexible
- Extract into
routingbut disable by default in the bitswap client -> could potentially cause problems for people updating without reading release notes, but would be much cleaner since then we could just leave this as a content routing wrapper - Extract the ProviderQueryManager into multiple different components for each of the jobs it does -> Might be a good idea, but can also happen later. Particularly if we make the changes in the near future any breakages people experience won't be significant
- Extract into https://github.com/libp2p/go-libp2p-routing-helpers -> I sort of wish this package was already moved into boxo (maybe it should be) and at the moment I'd rather not add more things to go-libp2p-routing-helpers that could reasonably fit in boxo unless downstreams want it to be separate (if you're a downstream and you care, here's where to let me know I'm wrong 😉)
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
P1High: Likely tackled by core team if no one steps upHigh: Likely tackled by core team if no one steps updif/expertExtensive knowledge (implications, ramifications) requiredExtensive knowledge (implications, ramifications) requiredkind/enhancementA net-new feature or improvement to an existing featureA net-new feature or improvement to an existing feature