Remove providing Exchange. Call Provide() from relevant places. #976
Conversation
See #963 Refactor the logic to perform Provides: * Remove providing Exchange() * Provide directly from Blockstore * Provide directly from pinner/merkledag on dag traversal * (Provides only happen when the component has been given a provider) One problem I have is that of circular dependencies between the `provider` module and the `blockstore/pinner/merkledag`. The main issue is that the provide interfaces are not separate from the reprovider implementation. For the moment I have copied the interfaces into the several modules to avoid this. MFS is untouched, for I believe we need to wrap the DagService as a "providing DAG service", or build a DAGService backed by a providing blockstore. It seems these can be done externally though.
hsanjuan
left a comment
There was a problem hiding this comment.
One problem I have is that of circular dependencies between the provider module and the blockstore/pinner/merkledag. The main issue is that the provide interfaces are not separate from the reprovider implementation. For the moment I have copied the interfaces into the several modules to avoid this.
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #976 +/- ##
==========================================
- Coverage 61.50% 61.41% -0.10%
==========================================
Files 253 254 +1
Lines 31573 31709 +136
==========================================
+ Hits 19420 19474 +54
- Misses 10565 10633 +68
- Partials 1588 1602 +14
... and 11 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
More problems when trying to integrate in Kubo:
This means we need
Need to think about it, potentially discuss in colo. |
|
The |
guillaumemichel
left a comment
There was a problem hiding this comment.
Nicely done!
MFS is untouched, for I believe we need to wrap the DagService as a "providing DAG service", or build a DAGService backed by a providing blockstore. It seems these can be done externally though.
So it means that this PR will (momentarily) break providing for mfs and pinned+mfs reprovide strategies. IMO it is fine to address this elsewhere.
IIUC the different reprovide strategies would require:
all: defineProvider()on the blockstoreflatdefineProvider()on the blockstore. Equivalent toallfor providing (but different priority during reprovides).roots: defineWithRootsProvider()on the pinnerpinned: defineWithPinnedProvider()on the pinnerpinned+mfs: defineWithPinnedProvider()on the pinner + TBDmfs: TBD
Is it correct?
Yes |
Fix small issues raised during review.
…rovider. The purpose here is to break import cycles. These methods forced pulling the pinner and the blockstore dependencies here. The latter was just a renaming of AllKeysChan and has been removed. The former has been moved to dspinner, along with a test. Blockstore provider is just a s
The providing strategy needs a blockstore, and the blockstore needs the provider system. Having a setter allows to break this cycle.
|
Ok:
With this I can get away with my intentions and I can: |
lidel
left a comment
There was a problem hiding this comment.
@hsanjuan thoughts on opening a Kubo PR that uses this to see the impact + be a place where we can start adding end-to-end tests for Reprovider.Strategy in Kubo?
Right now we lack test results that:
- confirm
Reprovider.Strategy=all|pinned|rootswork as expected - show
Reprovider.Strategy=mfs|pinned+mfsneed to be fixed
|
Triage notes:
|
The provider provides the CID every time something is added to the DAG service.
… any. Applies to Provide() calls in Blockstore, Merkledag, Pinner. Per PR review.
|
I have addressed comments and I have added a provider param to MFS constructors. This breaks the API but I don't know if it's worth to avoid that. Seems pretty straightforward to fix (passing nil, or a provider). In the end I have opted for carrying a Provider around in MFS because it matches how other modules do it and it's quite explicit, rather than doing a magic DAGService wrapper, or a mfs-only-providing-blockstore in Kubo. MFS calls Provide every time that there is a call to DAGService.Add, which seems enough. |
This updates boxo to this PR ipfs/boxo#976, which decentralizes the providing responsibilities. Then it perfoms the necessary changes in Kubo, which consist in initializing the Pinner, MFS and the blockstore with the provider.System. Since the provider.System is created first, the reproviding KeyChanFunc is set later when we can create it.
This updates boxo to this PR ipfs/boxo#976, which decentralizes the providing responsibilities. Then it perfoms the necessary changes in Kubo, which consist in initializing the Pinner, MFS and the blockstore with the provider.System. Since the provider.System is created first, the reproviding KeyChanFunc is set later when we can create it.
|
Review: make sure godoc reflects that you can pass And fix the tests. |
Done |
|
We will merge together with ipfs/kubo#10886 |
|
Per my latest advancements with Kubo, I think we're good to merge this here for the moment. |
* Provide according to strategy Updates boxo to a version with the changes from ipfs/boxo#976, which decentralize the providing responsibilities (from a central providing.Exchange to blockstore, pinner, mfs). The changes consist in initializing the Pinner, MFS and the blockstore with the provider.System, which is created first. Since the provider.System is created first, the reproviding KeyChanFunc is set later when we can create it once we have the Pinner, MFS and the blockstore. Some additional work applies to the Add() workflow. Normally, blocks would get provided at the Blockstore or the Pinner, but when adding blocks AND a "pinned" strategy is used, the blockstore does not provide, and the pinner does not traverse the DAG (and thus doesn't provide either), so we need to provide directly from the Adder. This is resolved by wrapping the DAGService in a "providingDAGService" which provides every added block, when using the "pinned" strategy. `ipfs --offline add` when the ONLINE daemon is running will now announce blocks per the chosen strategy, where before it did not announce them. This is documented in the changelog. A couple of releases ago, adding with `ipfs --offline add` was faster, but this is no longer the case so we are not incurring in any penalties by sticking to the fact that the daemon is online and has a providing strategy that we follow. Co-authored-by: gammazero <11790789+gammazero@users.noreply.github.com> Co-authored-by: Marcin Rataj <lidel@lidel.org>
See #963
Refactor the logic to perform Provides, when the component has been given a provider:
providing.ExchangeDAGService.AddAs an alternative to providing from MFS, wrapping the DagService as a "providing DAG service", or building a DAGService backed by a providing blockstore, was considered. It was decided that MFS should carry a Provider because it matches how other modules do it and it's quite explicit, MFS calls Provide every time that there is a call to DAGService.Add, which seems enough.