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.

Sign up for GitHub

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

Jump to bottom

Add ContentRoutingMany interface. #1758

Closed
wants to merge 1 commit into from
Closed

Add ContentRoutingMany interface. #1758

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions core/routing/routing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (

ci "github.com/libp2p/go-libp2p/core/crypto"
"github.com/libp2p/go-libp2p/core/peer"
"github.com/multiformats/go-multihash"

cid "github.com/ipfs/go-cid"
)
Expand Down Expand Up @@ -36,6 +37,16 @@ type ContentRouting interface {
FindProvidersAsync(context.Context, cid.Cid, int) <-chan peer.AddrInfo
}

// ContentRoutingMany complements ContentRouting intetface allowing providing several keys at
// the same time, improving performance.
type ContentRoutingMany interface {
// ProvideMany adds the given keys to the content routing system.
ProvideMany(ctx context.Context, keys []multihash.Multihash) error
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does the signature look so different from ContentRouting.Provide? I would have expected it to be identical to Provide, but just take a slice of CIDs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is historically coming from the FullRT DHT implementation: https://github.com/libp2p/go-libp2p-kad-dht/blob/master/fullrt/dht.go#L914

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ajnavarro some historical context. I used this signature for the DHT because it really only advertises multihashes and it seemed misleading (and incorrect) to use CIDs in the signature, also the broadcast boolean seemed very out of place ... why call Provide if you're not trying to broadcast?

That being said if it's useful to change ProvideMany's signature to something else (e.g. taking a slice of CIDs, taking a channel of CIDs, taking an iterator that yields CIDs, ...) that seems pretty reasonable.

Given that recently merged Provide methods for Reframe (https://github.com/ipfs/specs/blob/04dbe0267edbbc2c9a3a9df3540e71327fabf86d/reframe/REFRAME_KNOWN_METHODS.md#provide) take CIDs it might be more natural to switch to providing CIDs. This does mean taking a look at the DHT code to make sure it's not wasting a ton of memory as you do the conversion though and might help you figure out which signature is correct.


// Ready checks if the router is ready to start providing keys.
Ready() bool
Comment on lines +46 to +47
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems unrelated to bulk providing things, it's just that both functions were added to a particular struct (the accelerated DHT client)

}

// PeerRouting is a way to find address information about certain peers.
// This can be implemented by a simple lookup table, a tracking server,
// or even a DHT.
Expand Down