Skip to content
This repository has been archived by the owner on Jun 27, 2023. It is now read-only.

write blocks retrieved from the exchange to the blockstore #92

Merged
merged 9 commits into from
Jul 28, 2022

Conversation

MichaelMure
Copy link
Contributor

This follows the change in bitswap where that responsibility was removed (ipfs/go-bitswap#571).

@MichaelMure
Copy link
Contributor Author

cc @Jorropo

blockservice.go Outdated
@@ -326,6 +331,15 @@ func getBlocks(ctx context.Context, ks []cid.Cid, bs blockstore.Blockstore, fget

for b := range rblocks {
logger.Debugf("BlockService.BlockFetched %s", b.Cid())

// also write in the blockstore for caching
// TODO: for performance reason, it might make sense to batch those writes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably needs to happen before we merge (it has significant performance implications). A simple version would be to read from rblocks with a default select statement (i.e., drain it), then write a batch. That's not perfect, but it'll help as long as rblocks is buffered (IIRC, it is).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not perfect, but it'll help as long as rblocks is buffered (IIRC, it is).

Turns out, it's not buffered: https:/ipfs/go-bitswap/blob/b18a91d6023b83821c72253bbe5e37190db64d63/internal/getter/getter.go#L93

This follows the change in bitswap where that responsibility was removed.
@MichaelMure
Copy link
Contributor Author

I added the Put batching, tests to make sure that blockservice does write blocks, and followed the interface changes.

Again, we need to release and bubble up the dependency. I think the last remaining point is possibly to buffer the output channel in bitswap: #92 (comment)

blockservice.go Show resolved Hide resolved
@Jorropo
Copy link
Contributor

Jorropo commented Jul 27, 2022

Blocking in MichaelMure#1

@Jorropo
Copy link
Contributor

Jorropo commented Jul 28, 2022

The batch loop was too confusing, I rewrote it (hopefully) slightly better.

I would like a merge review on 30ca6aa pls.

Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, let's dog food this

@Jorropo Jorropo merged commit 275986e into ipfs:master Jul 28, 2022
@MichaelMure
Copy link
Contributor Author

I would like a merge review on 30ca6aa pls.

LGTM

@ajnavarro ajnavarro mentioned this pull request Aug 24, 2022
72 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants