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: pass records limit on routing.FindProviders #299

Merged
merged 3 commits into from
May 25, 2023
Merged

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented May 12, 2023

While working on ipfs/kubo#9877, I noticed that the ContentRouter interface required in order to implement the Routing V1 server does not provide enough information to make an education decision about how many providers to return.

This did not use to be an issue because the API was non-streamed so we could hardcode a value on the implementation (or timeout). Now that the server also supports streaming requests, it would be great to be able to differentiate both.

After talking to @lidel, we talked about adding a count int parameter in the FindProviders. By default, count will be 20 for non-streamable requests and 0 (unbounded) for streaming requests. However, I find that that introduces unnecessary complexity here. So I just added a stream bool parameter to FindProviders instead, and we let the implementer of ContentRouter decide what to do with it. See #299 (comment).

Note that existing tests that I updated test this too.

@hacdias hacdias requested review from lidel and Jorropo May 12, 2023 15:18
@hacdias hacdias self-assigned this May 12, 2023
@hacdias hacdias marked this pull request as ready for review May 12, 2023 15:19
@hacdias hacdias requested a review from a team as a code owner May 12, 2023 15:19
@codecov
Copy link

codecov bot commented May 12, 2023

Codecov Report

Merging #299 (a3c134b) into main (1daddd8) will decrease coverage by 0.57%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #299      +/-   ##
==========================================
- Coverage   48.15%   47.59%   -0.57%     
==========================================
  Files         279      276       -3     
  Lines       33524    33467      -57     
==========================================
- Hits        16145    15928     -217     
- Misses      15690    15849     +159     
- Partials     1689     1690       +1     
Impacted Files Coverage Δ
routing/http/server/server.go 65.38% <50.00%> (-1.63%) ⬇️

... and 19 files with indirect coverage changes

@hacdias hacdias changed the title feat: provide maximum count to routing.FindProviders feat: indicate if response will be streamable on routing.FindProviders May 12, 2023
@hacdias hacdias changed the base branch from main to fix/nljson-routing May 15, 2023 10:12
@hacdias hacdias force-pushed the fix/nljson-routing branch 2 times, most recently from f3bb207 to d37890a Compare May 15, 2023 10:33
@hacdias hacdias force-pushed the providers-count branch 2 times, most recently from 6af6415 to d96e912 Compare May 15, 2023 10:54
Base automatically changed from fix/nljson-routing to main May 16, 2023 12:51
@hacdias hacdias requested a review from lidel May 24, 2023 10:22
@hacdias hacdias changed the title feat: indicate if response will be streamable on routing.FindProviders feat: pass limit on routing.FindProviders May 24, 2023
@hacdias hacdias changed the title feat: pass limit on routing.FindProviders feat: pass records limit on routing.FindProviders May 24, 2023
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.

Configurable limit of returned responses lgtm, makes it more useful as a library, thanks!

@lidel lidel enabled auto-merge (squash) May 25, 2023 11:38
@lidel lidel merged commit a8533c9 into main May 25, 2023
@lidel lidel deleted the providers-count branch May 25, 2023 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants