Add ContentRoutingMany interface. #1758
Conversation
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.
| // 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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 |
There was a problem hiding this comment.
This seems unrelated to bulk providing things, it's just that both functions were added to a particular struct (the accelerated DHT client)
|
@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:
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? |
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
|
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 After adding this |
|
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).
This is a good example of the "many implementations" and "many callers". Any caller can of course redefine the
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. |
|
@ajnavarro what's the next step for this PR? Can it be closed? |
|
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. |
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.