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.

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

coreapi: DHT API #4804

Merged
merged 13 commits into from
Sep 11, 2018
Merged

coreapi: DHT API #4804

merged 13 commits into from
Sep 11, 2018

Conversation

magik6k
Copy link
Member

@magik6k magik6k commented Mar 10, 2018

Based on #4802

Replaces #4774

TODOs:

@magik6k magik6k requested a review from Kubuxu as a code owner March 10, 2018 18:35
@ghost ghost assigned magik6k Mar 10, 2018
@ghost ghost added the status/in-progress In progress label Mar 10, 2018
@magik6k magik6k added topic/core-api Topic core-api status/blocked Unable to be worked further until needs are met labels Mar 11, 2018
@magik6k magik6k mentioned this pull request Mar 12, 2018
51 tasks
@magik6k magik6k removed the status/blocked Unable to be worked further until needs are met label Mar 23, 2018
@magik6k magik6k requested review from a user and Stebalien March 24, 2018 22:23
@magik6k
Copy link
Member Author

magik6k commented Mar 24, 2018

#4807 should get merged first

@magik6k magik6k added the status/blocked Unable to be worked further until needs are met label Mar 24, 2018
This was referenced Mar 24, 2018
@Stebalien
Copy link
Member

@magik6k is this ready for review?

@magik6k
Copy link
Member Author

magik6k commented Mar 30, 2018

Rebased, should be ready for a review

@magik6k magik6k removed the status/blocked Unable to be worked further until needs are met label Mar 30, 2018
@Kubuxu
Copy link
Member

Kubuxu commented Mar 30, 2018

CI is red

@magik6k magik6k added the status/blocked Unable to be worked further until needs are met label Mar 30, 2018
@magik6k
Copy link
Member Author

magik6k commented Mar 30, 2018

I think there is a random test fail, will try to fix it first

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Any chance we could get the DHT commands converted over to use this API before merging? I understand if that would be too much too fast but this is a lot of unused code at the moment.

dht, ok := api.node.Routing.(*ipdht.IpfsDHT)
if !ok {
return nil, ErrNotDHT
}
Copy link
Member

Choose a reason for hiding this comment

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

Why not just call FindPeer directly on ipfs.node.Routing? The interface requires that method.

return
}

notif.PublishQueryEvent(ctx, &notif.QueryEvent{
Copy link
Member

Choose a reason for hiding this comment

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

Why not just call FindPeer, get the result, return it to the user, and be done with it? We're throwing away all the other events anyways.

return nil, err
}

dht, ok := api.node.Routing.(*ipdht.IpfsDHT)
Copy link
Member

Choose a reason for hiding this comment

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

Same here. We shouldn't be casting this. If we need some new functionality, we should add that to the interface.

defer close(events)
for p := range pchan {
np := p
notif.PublishQueryEvent(ctx, &notif.QueryEvent{
Copy link
Member

Choose a reason for hiding this comment

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

Again, maybe I'm missing something but this seems like pointless complexity. The caller of this method (FindProviders) can't even use these events because we're registering for (and consuming) them.

return errors.New("cannot provide in offline mode")
}

if len(api.node.PeerHost.Network().Conns()) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Oof. This is going to hurt. It's fine to do this from the command as we amortize over the request and multiple arguments but listing all the connections just to see if we have some connections is going to be very expensive. Ideally, the DHT would detect this and return an error. Does it not do that?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was it the dht command.

Looking at provide code in kad-dht, it invokes dht.GetClosestPeers, which I think will error in that case (correct me if I'm wrong) -> https://github.com/libp2p/go-libp2p-kad-dht/blob/2bab49ef85b2d8547e95ca2a699b5be562cc78b6/lookup.go#L60


//defer close(events)
if settings.Recursive {
err = provideKeysRec(ctx, api.node.Routing, api.node.DAG, []*cid.Cid{c})
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure this needs to be an offline DAG.

@@ -59,6 +59,11 @@ func (api *CoreAPI) Pin() coreiface.PinAPI {
return (*PinAPI)(api)
}

// Dht returns the DhtAPI interface implementation backed by the go-ipfs node
func (api *CoreAPI) Dht() coreiface.DhtAPI {
Copy link
Member

Choose a reason for hiding this comment

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

High level comment: I'd rather not have a DHT API. The DHT is just a backend for other stuff (the routing interface). While I know we currently have a command, I'd prefer not to bake this into the CoreAPI. Is there a design discussion for the CoreAPI anywhere?

Copy link
Member Author

@magik6k magik6k Mar 31, 2018

Choose a reason for hiding this comment

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

Is there a design discussion for the CoreAPI anywhere?

I'm trying to follow the js coreapi spec in most places - for DHT https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/DHT.md.

I did only include the basic functions which may be useful in some cases.

Copy link
Member

Choose a reason for hiding this comment

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

👍

I've submitted an issue to discuss this (https://github.com/ipfs/interface-ipfs-core/issues/249). We can probably move forward discuss this in parallel. It's not the end of the world, just a bit unfortunate.

}

func provideKeysRec(ctx context.Context, r routing.IpfsRouting, dserv ipld.DAGService, cids []*cid.Cid) error {
provided := cid.NewSet()
Copy link
Member

Choose a reason for hiding this comment

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

This is just a copy of the existing functionality but... it's going to hurt regardless. Could we get a TODO: Use a bloom filter?

Copy link
Member Author

Choose a reason for hiding this comment

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

for _, c := range cids {
kset := cid.NewSet()

err := dag.EnumerateChildrenAsync(ctx, dag.GetLinksDirect(dserv), c, kset.Visit)
Copy link
Member

Choose a reason for hiding this comment

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

Same, could we get a "TODO: provide and enumerate in parallel with a small buffer".

Copy link
Member Author

@magik6k magik6k Jul 19, 2018

Choose a reason for hiding this comment

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

Will get that with #4333

)

// DhtAPI specifies the interface to the DHT
type DhtAPI interface {
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather use the interface defined in https://github.com/libp2p/go-libp2p-routing/ (although we'd probably want to change it to return channels). Alternatively, we could do this for now with a big "we're going to change this" todo.

@magik6k magik6k force-pushed the feat/coreapi/dht branch 3 times, most recently from 36dec57 to 6665373 Compare March 31, 2018 00:29
@magik6k
Copy link
Member Author

magik6k commented Mar 31, 2018

Thanks for the review and pointers.

Any chance we could get the DHT commands converted over to use this API before merging

I guess it's probably better to get this done sooner than later. This is going to be at least a slightly breaking change. Currently when you run some dht command with --enc=json you get a really verbose output of everything the dht is doing. Imo it's weird to produce output that verbose in API, so I think it's fine to clean that up. Is there anything that depends on this output?

@magik6k
Copy link
Member Author

magik6k commented Aug 15, 2018

(should be ready to merge, though we might want one more review)

kevina pushed a commit to ipfs/go-cidutil that referenced this pull request Aug 16, 2018
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Two nits but nothing that we can't fix later (well, except the With prefix but I don't feel strongly about that).

errCh := make(chan error)
go func() {
for _, c := range cids {
dserv := dag.NewDAGService(blockservice.New(bs, offline.Exchange(bs)))
Copy link
Member

Choose a reason for hiding this comment

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

Let's construct this once.


// WithRecursive is an option for Dht.Provide which specifies whether to provide
// the given path recursively
func (dhtOpts) WithRecursive(recursive bool) DhtProvideOption {
Copy link
Member

Choose a reason for hiding this comment

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

What's the rational behind the With prefix?

Copy link
Member Author

Choose a reason for hiding this comment

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

Options before refractor used it, this didn't get updated with rebase. Will fix

@Stebalien
Copy link
Member

Hm. Actually, is there any way we could get some better test coverage or will that come when we switch commands over to using this interface?

@magik6k
Copy link
Member Author

magik6k commented Sep 1, 2018

coverage++ (73% on coreapi/dht.go)

(Circle appears to be borked/annoyingly slow - #5389 may or may not be related)

Copy link
Member

@Kubuxu Kubuxu left a comment

Choose a reason for hiding this comment

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

SGWM

magik6k and others added 13 commits September 10, 2018 17:33
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
License: MIT
Signed-off-by: Steven Allen <[email protected]>
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
@Stebalien Stebalien added the RFM label Sep 11, 2018
@Stebalien Stebalien merged commit eb45644 into master Sep 11, 2018
@ghost ghost removed the status/in-progress In progress label Sep 11, 2018
@Stebalien Stebalien deleted the feat/coreapi/dht branch February 28, 2019 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFM topic/core-api Topic core-api
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants