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(config): expose ProviderSearchMaxResults #10773

Merged
merged 1 commit into from
Apr 9, 2025
Merged

Conversation

lidel
Copy link
Member

@lidel lidel commented Apr 2, 2025

This PR is replacing hardcoded integer from #10765 with named default and expose config option for adjusting it, like we do in Rainbow.

@lidel lidel requested a review from hsanjuan April 2, 2025 23:04
@lidel lidel requested a review from a team as a code owner April 2, 2025 23:04
@@ -28,6 +28,7 @@ const (
DefaultEngineTaskWorkerCount = 8
DefaultMaxOutstandingBytesPerPeer = 1 << 20
DefaultProviderSearchDelay = 1000 * time.Millisecond
DefaultMaxProviders = 10 // matching BitswapClientDefaultMaxProviders from https://github.com/ipfs/boxo/blob/v0.29.1/bitswap/internal/defaults/defaults.go#L15
Copy link
Member Author

@lidel lidel Apr 2, 2025

Choose a reason for hiding this comment

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

Open question: should we keep 10, or switch to 0 (unlimited, like in rainbow?)

Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC MaxProviders in the context of bitswap is the max number of providers that Bitswap asks the content routing system (Amino DHT) to return, if more are returned only the first MaxProviders are read by Bitswap.

While it wouldn't be more effort for the content routing system to provide more providers (as long as they exist), we may want to bound the number of providers records bitswap/http will try to fetch the content from, for a single CID.

Also, with a MaxProviders that is too low, we may not be able to discover the content, if was advertised by many unstable nodes that have gone offline since.

Increasing DefaultMaxProviders to something like 32 could be a middle ground in ensuring content is likely to be found if there are online providers, while bounding retrieval attempts.

@hsanjuan
Copy link
Contributor

hsanjuan commented Apr 4, 2025

@gammazero do you remember if this made any difference at all?

@guillaumemichel
Copy link
Contributor

Triage notes:

  • We decided to keep the DefaultMaxProviders to 10 by default since 10 online providers should be enough.
  • Good to merge after CHANGELOG is added

@lidel lidel force-pushed the config-max-providers branch from 734a57b to 8e72cea Compare April 9, 2025 18:56
Replacing hardcoded integer with named default and expose
config option for adjusting it, like we do in Rainbow
https://github.com/ipfs/kubo/pull/10765/files#r2025455848
@lidel lidel force-pushed the config-max-providers branch from 8e72cea to 72c71a1 Compare April 9, 2025 19:01
@lidel lidel merged commit fe3106f into master Apr 9, 2025
16 checks passed
@lidel lidel deleted the config-max-providers branch April 9, 2025 19:17
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