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

feat: increase routing limits #154

Merged
merged 6 commits into from
Dec 2, 2024
Merged

Conversation

aschmahmann
Copy link
Contributor

@aschmahmann aschmahmann commented Aug 1, 2024

This builds off of ipfs/boxo#641 and at the moment has two notable changes from the current setup:

  1. The limit of in process lookups was raised from 6 to 100
  2. The limit on provider records to lookup was removed

Without some experimentation it's hard to tell what these numbers should be or what other sorts of heuristics we might want to use here. I don't think the ones we have are optimal, but for now figured we'd see if mostly tweaking the numbers helps at all. It's also not clear that we'd want both DHT and IPNI lookups to be rate-limited in the same way and right now they're joined together.

We should try this out on a staging branch and see what the impact looks like.

@aschmahmann aschmahmann force-pushed the feat/increase-routing-limits branch from 36fbef7 to adc6960 Compare August 1, 2024 20:59
@lidel lidel added the status/blocked Unable to be worked further until needs are met label Sep 3, 2024
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Triage note: this is waiting for results of staging A/B testing results.

Note2: test after rebase on bitswap/routing improvements

@gammazero gammazero self-assigned this Nov 25, 2024
@hsanjuan hsanjuan force-pushed the feat/increase-routing-limits branch from 0693583 to 2ebfc5b Compare November 27, 2024 13:32
@hsanjuan
Copy link
Contributor

This is ready for testing I think

@gammazero
Copy link
Contributor

I am seeing TestSeedPeering failing when running tests locally.

@hsanjuan hsanjuan removed the status/blocked Unable to be worked further until needs are met label Nov 29, 2024
if err != nil {
panic(err)
}
pqm.Startup()
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless there is a need to start at some later time, or some modes of operation need a ProviderQueryQanager that has not been started, then I think New should also perform the startup.

Copy link
Contributor

Choose a reason for hiding this comment

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

Doing so prevents problems where calling Startup is missed.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gammazero gammazero merged commit 10cd50a into main Dec 2, 2024
11 checks passed
@gammazero gammazero deleted the feat/increase-routing-limits branch December 2, 2024 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants