-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add options for record count and timeout for resolving DHT paths #4733
Conversation
License: MIT Signed-off-by: Dirk McCormick <[email protected]>
This PR adds two new parameters to Currently when resolving 16 IPNS records will be retrieved from the DHT (where the DHT k-value is 20). The Currently the timeout for retrieving a public key for an IPNS record is one minute, and the timeout for retrieving the value itself is one minute. The public key and value are retrieved sequentially so it can take up to 2 minutes to time out. The |
License: MIT Signed-off-by: Dirk McCormick <[email protected]>
@vyzo @Stebalien @whyrusleeping could I ask you guys to take a look at this? |
core/commands/ipns.go
Outdated
opts.Depth = 1 | ||
} | ||
if rcok { | ||
opts.DhtRecordCount = uint(rc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can get rid of this cast once ipfs/go-ipfs-cmdkit#14 is approved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to make a new method in namesys that takes the parameters, so that you don't have to rewrite all the references.
My concern was that we would then have three different methods for doing the same thing. Do you think that's not worth worrying about? |
My suggestion was more to add the new method that takes the options, and redirect the old method to the new one with default options. That way you wouldn't change the existing code at all. |
I understand, my concern is about having three different methods in the API where we only really need one. Is that not worth worrying about? |
Hrm, dunno -- it's nice to have a simple method that takes default options. |
When in doubt, variadic options can be a good interface |
Yes you're (both) right, that is nicer, I'll use variadic options |
License: MIT Signed-off-by: Dirk McCormick <[email protected]>
License: MIT Signed-off-by: Dirk McCormick <[email protected]>
License: MIT Signed-off-by: Dirk McCormick <[email protected]>
Ok changed to use variadic options, let me know if that makes sense |
} | ||
|
||
if !strings.HasPrefix(name, "/ipns/") { | ||
name = "/ipns/" + name | ||
} | ||
|
||
output, err := resolver.ResolveN(req.Context(), name, depth) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where did the depth arg go?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh nvm, i found it
core/coreapi/name.go
Outdated
if !strings.HasPrefix(name, "/ipns/") { | ||
name = "/ipns/" + name | ||
} | ||
|
||
output, err := resolver.ResolveN(ctx, name, depth) | ||
ropts := []nsopts.ResolveOpt{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i prefer writing this as:
var ropts []nsopts.ResolveOpt
License: MIT Signed-off-by: Dirk McCormick <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is the options package a separate package?
It would be nicer from a usability perspective if it was inside namesys
so that we wouldn't have to import it separately.
other than that LGTM.
I put it in a separate package as a way of namespacing, so that you call |
Fair enough -- not really an issue :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, using Opts pattern fits here very well.
core/commands/ipns.go
Outdated
@@ -57,9 +59,10 @@ Resolve the value of a dnslink: | |||
Options: []cmdkit.Option{ | |||
cmdkit.BoolOption("recursive", "r", "Resolve until the result is not an IPNS name."), | |||
cmdkit.BoolOption("nocache", "n", "Do not use cached entries."), | |||
cmdkit.UintOption("dht-record-count", "dhtrc", "Number of records to request for DHT resolution."), | |||
cmdkit.UintOption("dht-timeout", "dhtt", "Timeout in seconds for DHT resolution. Pass 0 for no timeout."), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I might be missing something but IPFS commands have a timeout flag (e.g., ipfs --timeout=30s
). And we can always set a DHT resolution timeout by setting a timeout on the context. What's the usecase for the additional timeout?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use case is that with --dht-timeout the DHT collects as many values as it can within the timeout, and then returns the best value it has found. The global IPFS command --timeout will simply kill the command:
> ./cmd/ipfs/ipfs name resolve --timeout=5s QmSgxPeqLrnM1ZB1pno3tYMQfqLB4cWXuAQNfSWsk77apK
Error: Post http://127.0.0.1:5001/api/v0/name/resolve?arg=QmSgxPeqLrnM1ZB1pno3tYMQfqLB4cWXuAQNfSWsk77apK&encoding=json&stream-channels=true&timeout=5s: context deadline exceeded
> ./cmd/ipfs/ipfs name resolve --dhtt=5 QmSgxPeqLrnM1ZB1pno3tYMQfqLB4cWXuAQNfSWsk77apK
/ipfs/QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it would be better for the --dht-timeout
parameter to take a string though, eg --dht-timeout=5s
instead of assuming seconds eg --dht-timeout=5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. It would be nice to explain that in the help but I can't think of a succinct way of putting it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dirkmc it will be better for it to parse the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a commit to parse the time, and changed the parameter description to
Max time to collect values during DHT resolution eg \"30s\". Pass 0 for no timeout
That's the most succinct way I can think of to say it
…olve License: MIT Signed-off-by: Dirk McCormick <[email protected]>
License: MIT Signed-off-by: Dirk McCormick <[email protected]>
b275d7b
to
494f242
Compare
if err != ErrExpiredRecord { | ||
t.Fatal("ValidateIpnsRecord should have returned ErrExpiredRecord") | ||
_, err = resolver.resolveOnce(ctx, id.Pretty(), opts.DefaultResolveOpts()) | ||
if err == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why arent we checking for a specific error anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The resolveOnce()
method used to call routing.GetValue()
but now we call routing.GetValues()
directly.
The mock ValueStore
implementation's GetValue()
method would get a single record and call VerifyRecord()
on it, which would return ErrExpiredRecord
/ErrSignature
etc. We now call the mock ValueStore
implementation's GetValues() method, which gets multiple records, calls VerifyRecord()
on each one individually, and adds the record to the list of valid records if there's no error. If there are no valid records it returns routing.ErrNotFound
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into whether I can check for routing.ErrNotFound
but the go-ipfs-routing mock returns datastore.ErrNotFound
instead of routing.ErrNotFound
when it doesn't find a key:
https://github.com/ipfs/go-ipfs-routing/blob/master/mock/centralized_client.go#L46
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, that seems fine. Thanks for the response!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just one question on a test decision, then i'll merge.
Thanks @dirkmc :)
Add options for record count and timeout for resolving DHT paths This commit was moved from ipfs/kubo@41d82ee
PR for issue #4723
EDIT: ~Kubuxu
Fixes #4723