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.

Sign up for GitHub

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

Jump to bottom

Provider query manager improvements #716

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Provider query manager improvements #716

wants to merge 15 commits into from
+302 −170

Conversation

gammazero
Copy link
Contributor

@gammazero gammazero commented Nov 12, 2024 •
edited
Loading

  • Use atomic value to protect variable timeout.
  • Remove inProgressRequestStatuses map when empty.
  • Do not append to slice forever. Use type that reuses items removed from front of list (e.g. channelqueue, deque
  • Add configurable bitswap options:
    • WithMaxConcurrentFinds
    • WithMaxProvidersPerFind
  • Make bitswap options configurable through client.

@gammazero gammazero requested a review from a team as a code owner November 12, 2024 10:47
@codecov Codecov
Copy link

codecov bot commented Nov 12, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 78.48101% with 34 lines in your changes missing coverage. Please review.

Project coverage is 60.39%. Comparing base (c91cc1d) to head (9326f0a).

Files with missing lines Patch % Lines
bitswap/client/optios.go 56.52% 20 Missing ⚠️
...ernal/providerquerymanager/providerquerymanager.go 86.76% 6 Missing and 3 partials ⚠️
...ap/client/internal/providerquerymanager/options.go 83.87% 3 Missing and 2 partials ⚠️

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #716      +/-   ##
==========================================
- Coverage   60.43%   60.39%   -0.05%     
==========================================
  Files         243      245       +2     
  Lines       31059    31111      +52     
==========================================
+ Hits        18771    18788      +17     
- Misses      10628    10654      +26     
- Partials     1660     1669       +9
Files with missing lines Coverage Δ
bitswap/client/client.go 90.25% <100.00%> (+2.31%) ⬆️
bitswap/server/internal/decision/engine.go 91.48% <100.00%> (ø)
...ap/client/internal/providerquerymanager/options.go 83.87% <83.87%> (ø)
...ernal/providerquerymanager/providerquerymanager.go 87.73% <86.76%> (-3.59%) ⬇️
bitswap/client/optios.go 56.52% <56.52%> (ø)

... and 9 files with indirect coverage changes

🚨 Try these New Features:

@lidel lidel requested a review from hsanjuan November 12, 2024 17:37
@hsanjuan
Copy link
Contributor

hsanjuan commented Nov 15, 2024 •
edited
Loading

@gammazero heads up that I'm not looking into this one until #641 is through, at which point it will need to be redone...

@gammazero
Provider query manager improvements
64604d3 
- Replace Debugf with Debugw to avoid unnecessary formatting whn no debug logging.
- Use atomic value to protect changable timeout.
- Remove inProgressRequestStatuses map when empty.
- Do not append to slice forever. Use Deque that reuses items removed from front of slice.
@gammazero
mod tidy examples
39caa17
@gammazero
update deque
9cb975f
@gammazero
mod tidy examples
f65d610
@gammazero
finish update
a351504
@gammazero
Use const for unfreesePeerInterval
351b8a9
@gammazero
providerQueryMessages should be unbuffered, confirmed by testing
3e11c1f
@gammazero
Replace providerRequestBufferWorker with channelqueue
f3dd35d 
Add configurable options:
- WithMaxInProcessRequests
- WithMaxProvidersPerFind
@gammazero
Make ProviderQueryManager options configurable through client
853b3ce
@gammazero
Rename maxInProcessRequests to maxConcurrentFinds
4b48c08
@gammazero
rename options
f6d672a
@gammazero gammazero mentioned this pull request Nov 22, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hsanjuan hsanjuan Awaiting requested review from hsanjuan

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
skip/changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gammazero @hsanjuan