Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove requirement on ProviderQueryManager from the bitswap client #640

Closed
aschmahmann opened this issue Jul 25, 2024 · 0 comments · Fixed by #641
Closed

Remove requirement on ProviderQueryManager from the bitswap client #640

aschmahmann opened this issue Jul 25, 2024 · 0 comments · Fixed by #641
Assignees
Labels
dif/expert Extensive knowledge (implications, ramifications) required kind/enhancement A net-new feature or improvement to an existing feature P1 High: Likely tackled by core team if no one steps up

Comments

@aschmahmann
Copy link
Contributor

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:

  1. Have max timeouts
  2. Have predetermined max provider records to use
  3. Does preconnects before returning the peers
  4. Deduplicates ongoing queries
  5. 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 FindProvidersAsync method 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:

  1. If these wrappers are needed/useful elsewhere they can be
  2. 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
  3. 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 routing but 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 😉)
@aschmahmann aschmahmann added kind/enhancement A net-new feature or improvement to an existing feature need/triage Needs initial labeling and prioritization labels Jul 25, 2024
@gammazero gammazero added P1 High: Likely tackled by core team if no one steps up dif/expert Extensive knowledge (implications, ramifications) required and removed need/triage Needs initial labeling and prioritization labels Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dif/expert Extensive knowledge (implications, ramifications) required kind/enhancement A net-new feature or improvement to an existing feature P1 High: Likely tackled by core team if no one steps up
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants