Codecov Report

Attention: Patch coverage is 81.09453% with 38 lines in your changes missing coverage. Please review.

Project coverage is 60.33%. Comparing base (c91cc1d) to head (8b304f2).

@@            Coverage Diff             @@
##             main     #641      +/-   ##
==========================================
- Coverage   60.43%   60.33%   -0.11%     
==========================================
  Files         243      244       +1     
  Lines       31059    31034      -25     
==========================================
- Hits        18771    18724      -47     
- Misses      10628    10641      +13     
- Partials     1660     1669       +9     

... and 7 files with indirect coverage changes

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

@aschmahmann Does this mean we still want the provider query manager, and need to close #536 which removes it.

lol I meant to just comment, not approve my own changes

Sync with @aschmahmann :

• ProviderQueryManager looks a lot like a ContentRouter itself.
• Hector proposes removing the ContentRouter (CR) part from BitSwapNetwork and passing it directly to Bitswap.New(). This is a breaking change (people need to pass CR to Bitswap.New() rather than to the bsnet.New().
• Adin wants to make ContentRouter completely optional in Bitswap Client, after removing from Server. That provides further justification to break bsnet and Bitswap.New() behaviour.
• When ContentRouter is provided, it might already be a disguised ProviderQueryManager, so users could:
  • Use no ContentRouting
  • Use their custom ContentRouting
  • Use ProviderQueryManager
  • This means users would need to remember to manually create ProviderQueryManager on upgrade, to keep things working as before. That's shitty and they will likely forget.
  • Alternatively bitswap can:
    • Receive a ContentRouting
    • Also have an option about whether that content routing should be wrapped in a default ProviderQueryManager or not. Defaulting to yes.

I think this is the gist @aschmahmann . I think the last option would be ok (there's breakage but it both a) has a stronger reason b) is easy to fix.

I think the last option would be ok (there's breakage but it both a) has a stronger reason b) is easy to fix.

Do you mean to do this?

1. Remove the Providing out of the Bitswap server (e.g. something like Move providing responsabilities from bitswap to blockservice #677)
2. Break the bitswap.Network by removing the ContentRouting
3. Add two options to bitswap.Client, 1 for adding a ProviderFinder and another for whether the default ProviderQueryManager should be used

Existing consumers would migrate by moving their ContentRouter from bitswap.Network into the bitswap.Client option

If so, that sounds reasonable to me. If you think pulling out the Providing is too much to do here we could also do something like:

1. Break the bitswap.Network by removing the ContentRouting
2. Add two options to bitswap.Bitswap, 1 for adding a ContentRouting and another (in bitswap.Client) for whether the default ProviderQueryManager should be used
   ...
   In a follow up PR:
3. Remove the Providing out of the Bitswap server
4. Change the bitswap.Bitswap option to forward to a bitswap.Client option and pass a ProviderFinder instead of a ContentRouter (is just a subset so nothing should break)

@aschmahmann @gammazero I have a question: I was going to port the "Provider" logic from bitswap server, with its workers and queues and what not. Then I saw that we have a provider.System (the reprovider) that does have queues, does batching and even stores pending provides to disk. Naively my thought is that, if such logic is already implemented in the reprovider, a providing.Exchange should not yet have its own way and we should just pass-in the reprovider when using it. Plus I suspect that we make use of things like Accelerated-DHT-providing, ProvideMany, so why going around...

What do you think?

What do you think?

Makes sense to me to leave it out. It's unclear how useful the parallel queues were beforehand, but it seems like the kind of thing that's situational enough that people can choose to add whatever queues they need and we should start off keeping it simple and encouraging reuse of the existing queue.

ipfs/kubo#10595 is testing this.