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

change the Pinner interface to have async pin listing #24

Closed
wants to merge 2 commits into from

Conversation

MichaelMure
Copy link

@MichaelMure MichaelMure commented Jan 20, 2023

The rational is that if the pin list get big, a synchronous call to get the complete list can delay handling unnecessarily. For example, when listing indirect pins, you can start walking the DAGs immediately with the first recursive pin instead of waiting for the full list.

This matters even more on low power device, of if the pin list is stored remotely.

Corresponding Kubo PR: ipfs/kubo#9692

@lidel lidel requested a review from Jorropo February 27, 2023 14:52
MichaelMure added a commit to MichaelMure/go-ipfs-provider that referenced this pull request Mar 2, 2023
MichaelMure added a commit to MichaelMure/go-ipfs-provider that referenced this pull request Mar 3, 2023
MichaelMure added a commit to MichaelMure/go-ipfs-provider that referenced this pull request Mar 3, 2023
MichaelMure added a commit to MichaelMure/go-ipfs-provider that referenced this pull request Mar 3, 2023
MichaelMure added a commit to MichaelMure/go-ipfs-provider that referenced this pull request Mar 3, 2023
@MichaelMure
Copy link
Author

Corresponding Kubo PR: ipfs/kubo#9692

@@ -3,6 +3,7 @@ module github.com/ipfs/go-ipfs-pinner
go 1.19

require (
github.com/Jorropo/channel v0.0.0-20230303124104-2821e25e07ff

Choose a reason for hiding this comment

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

can we not add this dependency?

Choose a reason for hiding this comment

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

If we want to return a union of "val or err" from a chan, then it's perfectly idiomatic to return a struct { Pin coreiface.Pin, Err error}, I'd prefer not to add another module to our rat's nest of dependencies for something that already has an established idiom.

Copy link

Choose a reason for hiding this comment

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

has an established idiom

I don't belive that true, most code in the std is synchronous or uses methods of interface who secretly manage synchronisation.

Choose a reason for hiding this comment

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

We don't need a dependency for this. Please don't do it

@@ -119,14 +120,14 @@ type Pinner interface {
Flush(ctx context.Context) error

// DirectKeys returns all directly pinned cids
DirectKeys(ctx context.Context) ([]cid.Cid, error)
DirectKeys(ctx context.Context) channel.ReadOnly[cid.Cid]
Copy link
Author

Choose a reason for hiding this comment

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

Suggested change
DirectKeys(ctx context.Context) channel.ReadOnly[cid.Cid]
type StreamedCid struct {
C cid.Cid
Err error
}
DirectKeys(ctx context.Context) chan StreamedCid

MichaelMure added a commit to MichaelMure/boxo that referenced this pull request May 4, 2023
The rational is that if the pin list get big, a synchronous call to get the complete list can delay handling unnecessarily. For example, when listing indirect pins, you can start walking the DAGs immediately with the first recursive pin instead of waiting for the full list.

This matters even more on low power device, of if the pin list is stored remotely.

Replace:
- ipfs/go-ipfs-pinner#24
- ipfs/go-ipfs-provider#48
@MichaelMure MichaelMure closed this May 5, 2023
Jorropo pushed a commit to MichaelMure/boxo that referenced this pull request Jun 2, 2023
The rational is that if the pin list get big, a synchronous call to get the complete list can delay handling unnecessarily. For example, when listing indirect pins, you can start walking the DAGs immediately with the first recursive pin instead of waiting for the full list.

This matters even more on low power device, of if the pin list is stored remotely.

Replace:
- ipfs/go-ipfs-pinner#24
- ipfs/go-ipfs-provider#48
Jorropo pushed a commit to MichaelMure/boxo that referenced this pull request Jun 2, 2023
The rational is that if the pin list get big, a synchronous call to get the complete list can delay handling unnecessarily. For example, when listing indirect pins, you can start walking the DAGs immediately with the first recursive pin instead of waiting for the full list.

This matters even more on low power device, of if the pin list is stored remotely.

Replace:
- ipfs/go-ipfs-pinner#24
- ipfs/go-ipfs-provider#48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants