go-libp2p
go-libp2p copied to clipboard
libp2p
Add ContentRoutingMany interface.
We had been using the ProvideMany method in some Routing implementations to improve performance.
We had several places where we needed to create this same interface to cast Routers to check if they support the ProvideMany method. To avoid that, and to have a common source of truth we think we should make this interface live with the other Routing ones.
@ajnavarro : how critical is this change for the work you're doing for Kubo 0.16? Just trying to get a gage on priority.
Not critical, but we have the same interface in several places:
- https://github.com/libp2p/go-libp2p-kad-dht/blob/dae5a9a5bd9c7cc8cfb5073c711bc308efad0ada/fullrt/dht.go
- https://github.com/ipfs/kubo/blob/b8412efb80f1205be70dd71cfca64c68456682e8/routing/delegated.go
- https://github.com/ipfs/go-delegated-routing/blob/0c2a9b60cb8e145c333d6ec33dd7dc8438c8277f/client/contentrouting.go
- https://github.com/ipfs/go-ipfs-provider/blob/4aff05e6304c6e222c4ff7ebbb6c6f8df6d8aa17/batched/system.go
And I thought that was something that we should unify somehow.
@ajnavarro : I saw you closed the PR. Are you closing it because of my comments or because of the comments from others? If the comments from others is causing you to think this isn't needed then that is fine. My question was coming from a place of wanting to understand how to prioritize libp2p maintainer reviewer time.
Reopening this because is a problem that we still have, even if we need a better solution for it.
I'm not sure I fully understand the value of adding the interface here. In general, Go has the callers define the interface: https://dave.cheney.net/practical-go/presentations/gophercon-israel.html#_let_callers_define_the_interface_they_require. This is (imo) the best feature of Go. So, with that context, what's the value of putting this interface here?
I'm not sure I fully understand the value of adding the interface here.
The value is to have one source of truth where all implementations can validate their API contract. This is the same reason to have the actual existing Routing interfaces here.
The actual ContentRoutingMany proposed interface is not the best one, but is the actual one that is being used on many Router implementations:
- https://github.com/libp2p/go-libp2p-kad-dht/blob/dae5a9a5bd9c7cc8cfb5073c711bc308efad0ada/fullrt/dht.go
- https://github.com/ipfs/kubo/blob/b8412efb80f1205be70dd71cfca64c68456682e8/routing/delegated.go
- https://github.com/ipfs/go-delegated-routing/blob/0c2a9b60cb8e145c333d6ec33dd7dc8438c8277f/client/contentrouting.go
- https://github.com/ipfs/go-ipfs-provider/blob/4aff05e6304c6e222c4ff7ebbb6c6f8df6d8aa17/batched/system.go
- https://github.com/libp2p/go-libp2p-routing-helpers/blob/master/compparallel.go#L44
- https://github.com/libp2p/go-libp2p-routing-helpers/blob/master/compsequential.go#L37
I'm not sure I fully understand the value of adding the interface here.
The value is to have one source of truth where all implementations can validate their API contract. This is the same reason to have the actual existing Routing interfaces here.
The actual
ContentRoutingManyproposed interface is not the best one, but is the actual one that is being used on many Router implementations:* https://github.com/libp2p/go-libp2p-kad-dht/blob/dae5a9a5bd9c7cc8cfb5073c711bc308efad0ada/fullrt/dht.go * https://github.com/ipfs/kubo/blob/b8412efb80f1205be70dd71cfca64c68456682e8/routing/delegated.go * https://github.com/ipfs/go-delegated-routing/blob/0c2a9b60cb8e145c333d6ec33dd7dc8438c8277f/client/contentrouting.go * https://github.com/ipfs/go-ipfs-provider/blob/4aff05e6304c6e222c4ff7ebbb6c6f8df6d8aa17/batched/system.go * https://github.com/libp2p/go-libp2p-routing-helpers/blob/master/compparallel.go#L44 * https://github.com/libp2p/go-libp2p-routing-helpers/blob/master/compsequential.go#L37
Do all of those callers use all the methods of the interface? If so, it sounds like you may want a common interface, but I don't see why that should be in go-libp2p (as opposed to any other common repo). And if not, then it probably makes the most sense to have each of these callers define their interface.
Do all of those callers use all the methods of the interface? If so, it sounds like you may want a common interface, but I don't see why that should be in go-libp2p (as opposed to any other common repo). And if not, then it probably makes the most sense to have each of these callers define their interface.
Hi Marco!
These are implementations of the different Routing interfaces, Also implementing the ContentRoutingMany interface. These routing interfaces must be on libp2p because are defining the contract with some libp2p features, (you can see different places where PeerRouting, ValueStore, ContentRouting are used, like all Reader and Writer interfaces are on io package)
After adding this ContentRoutingMany interface, it can be used on libp2p internally to improve performance.
I took some time to review this issue from the top again, and I think I understand it better now. We have multiple implementations of this "ProvideMany" interface across different repos. And the reason to want to merge this interface into go-libp2p is that it's the common dependency across these repos and could be used to assert that these implementations are all implementing the same interface. Is that correct?
How many callers are there? Is this many implementations but only 1 caller (for now)?
If so, this should still live with the caller. It's the only one that cares about this interface.
If there is more than 1 caller (forgive me if I missed something, I only found this caller - not including the composed callers) then this interface should probably live in the most common caller (i.e. the common dependency).
like all Reader and Writer interfaces are on io package
This is a good example of the "many implementations" and "many callers". Any caller can of course redefine the io.Reader interface, but it's commonly used because it's convenient to have callers and implementations agree on what a basic "Reader" looks like. But note that the io package itself uses Reader. The Reader interface isn't only defined in the io package as convenience for other packages, but because it itself uses it. The io package cares about this interface as a caller. And, as its role in one of the core packages, exposes this interface to help many callers and many implementations agree on what a Reader is.
After adding this ContentRoutingMany interface, it can be used on libp2p internally to improve performance.
As expanded on above, I think this is backwards. go-libp2p should need this interface before we define it. Right now go-libp2p itself doesn't use this interface. It isn't a caller so why would it care about this interface?
If we make changes such that go-libp2p does use this interface, then yes. Let's bring in the interface. But those changes have to come before we define this interface.
I think there can be exceptions to the rules above, but I think it's important to think carefully about them.
We can close this PR.
There is no agreement about having a ProvideMany interface to be in use internally by go-libp2p (which will heavily improve performance when providing more than a few CIDs) and making this the source of truth for routing implementations.