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

bitswap/server: allow overriding peer ledger with WithPeerLedger #607

Merged
merged 4 commits into from
Apr 26, 2024
Merged

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Apr 25, 2024

Introduces WithPeerLedger, which allows to override the default peer ledger with anything else you want.

@hacdias hacdias requested a review from a team as a code owner April 25, 2024 14:16
Copy link

codecov bot commented Apr 25, 2024

Codecov Report

Attention: Patch coverage is 63.33333% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 59.69%. Comparing base (eeea414) to head (8d74823).

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #607      +/-   ##
==========================================
- Coverage   59.88%   59.69%   -0.20%     
==========================================
  Files         238      238              
  Lines       29868    29883      +15     
==========================================
- Hits        17887    17839      -48     
- Misses      10375    10425      +50     
- Partials     1606     1619      +13     
Files Coverage Δ
bitswap/server/internal/decision/peer_ledger.go 94.17% <100.00%> (-1.79%) ⬇️
bitswap/options.go 38.09% <0.00%> (-1.91%) ⬇️
bitswap/server/internal/decision/engine.go 91.22% <20.00%> (-0.68%) ⬇️
bitswap/server/server.go 64.63% <0.00%> (-2.43%) ⬇️

... and 16 files with indirect coverage changes

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.

I don't feel strongly how we name it, but

  • feels like we could go away with adding just one option for this
  • rainbow should use explicit noopPeerLedger that has all methods implemented as noop. this will be less prone to breakage / bugs during refactors.

details inline

bitswap/server/internal/decision/engine.go Outdated Show resolved Hide resolved
bitswap/server/internal/decision/engine.go Outdated Show resolved Hide resolved
@hacdias hacdias changed the title bitswap/server: options WithNotifyNewBlocks and WithNoPeerLedger bitswap/server: allow overriding peer ledger with WithPeerLedger Apr 26, 2024
@hacdias
Copy link
Member Author

hacdias commented Apr 26, 2024

@lidel I refactored to only have one new option (WithPeerLedger). I did not add a no-op peer ledger here. I will do this in Rainbow. My idea is that I didn't want to export custom peer ledgers here. We also do not export the default peer ledger, just like it has been done with the score ledger. Just trying to avoid inconsistencies here. If we want to use the no-op peer ledger in other places, we can then bring it to Boxo.

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.

Lgtm, follows what we already had for WithScoreLedger and does not introduce anything new.


Digression on future work:
We are still figuring out what appropriate settings for rainbow are, but once we do, it will be useful for others. If we want to add a higher abstraction similar to what @aschmahmann suggested (WithRequestResponseOnly), I feel it needs to be more generic, e.g.: WithProfile("minimal-request-response") because it would adjust multiple settings, and work similar to Kubo profiles.

@hacdias hacdias merged commit 0f223aa into main Apr 26, 2024
15 checks passed
@hacdias hacdias deleted the bs branch April 26, 2024 12:07
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.

3 participants