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

Update otlp #529

Closed
wants to merge 5 commits into from
Closed

Update otlp #529

wants to merge 5 commits into from

Conversation

Jorropo
Copy link
Contributor

@Jorropo Jorropo commented Dec 29, 2023

Based on #528

We always had a very weird relationship between bitswap and providing.
Bitswap took care of doing the initial provide and then reprovider did it later.
The Bitswap server had a complicated providing workflow where it slurped thing into memory.

Reprovide accepts provides and is able to queue them in a database, such as on disk, this is much better.

I'll add options to hook initial provide logic from the blockservice to the reprovider queue so consumers don't have to do this themselves.
…bilities to an option of the client

Given that the previous commit remove the content advertising from the server, it did not made sense to share these paths on the network.

The code has been reworked:
- addresses aren't magically added to the peerstore as a side-effect of calling `Network.FindProvidersAsync`. Instead they are passed as hints to ConnectTo which copies libp2p `host.ConnectTo` API.
- the providerquerymanager is completely shutdown when not using `WithContentSearch` option, this helps usecase where `routinghelpers.Null` is used for content routing and the consumer exclusively rely on broadcast, like networks where most peoples have all the content (Filecoin, Celestia, ...).
This allows to recreate the behavior of advertising added blocks the bitswap server used to do.
blockservice is explicitely tolerent to having a nil exchange.
The constructor even logs that as running an offline blockservice.

Everything is except close, which panics.

It is confusing for consumers to only have to call close based on if it's online or offline.
They could also instead call close directly on the exchange (then we could remove blockservice's Close method).

Anyway here is as a simple fix, add a nil check.
@Jorropo Jorropo requested a review from a team as a code owner December 29, 2023 08:00
Copy link

codecov bot commented Dec 29, 2023

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (483bc39) 65.62% compared to head (fda54dc) 65.51%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #529      +/-   ##
==========================================
- Coverage   65.62%   65.51%   -0.11%     
==========================================
  Files         207      207              
  Lines       25549    25445     -104     
==========================================
- Hits        16766    16670      -96     
+ Misses       7317     7302      -15     
- Partials     1466     1473       +7     
Files Coverage Δ
bitswap/bitswap.go 69.51% <ø> (+2.07%) ⬆️
bitswap/client/client.go 89.79% <100.00%> (-0.63%) ⬇️
...tswap/client/internal/messagequeue/messagequeue.go 83.53% <ø> (-1.39%) ⬇️
bitswap/client/internal/session/session.go 91.20% <100.00%> (+0.54%) ⬆️
bitswap/network/ipfs_impl.go 78.86% <100.00%> (+2.27%) ⬆️
bitswap/options.go 44.44% <100.00%> (ø)
bitswap/server/server.go 59.75% <100.00%> (-6.72%) ⬇️
bitswap/testnet/peernet.go 38.46% <100.00%> (-4.40%) ⬇️
blockservice/test/mock.go 100.00% <100.00%> (ø)
examples/unixfs-file-cid/main.go 41.21% <100.00%> (ø)
... and 4 more

... and 6 files with indirect coverage changes

@Jorropo Jorropo closed this Dec 29, 2023
@Jorropo Jorropo deleted the update-otlp branch December 29, 2023 10:39
@Jorropo
Copy link
Contributor Author

Jorropo commented Dec 29, 2023

Will do in #528

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.

1 participant