go-libp2p-kad-dht: implement GetValuesAsync #49
Conversation
mildred
force-pushed
the
get-values-async
branch
from
ef6d5fe to
f3d4e75
Compare
| return vals, nil | ||
| } | ||
|
|
||
| func (dht *IpfsDHT) GetValuesAsync(ctx context.Context, key string, nvals int) <-chan *routing.RecvdVal { |
There was a problem hiding this comment.
whyrusleeping:
Maybe we want to return a channel of 'maybe values' that have either a RecvdVal or an error? Might make the handling of errors a bit simpler
mildred:
Yes, perhaps, but I didn't know so I took the same approach as FindProvidersAsync. I'm ready to change that if you think so.
mildred/ipfs-objects@39c20b2#diff-50d2a8e6d11e785b103932ac6c926e72R169
There was a problem hiding this comment.
You added the error field into the RecvdVal in the routing interface PR. Should we make use of it?
There was a problem hiding this comment.
Ahh, I see getValuesAsyncRoutine will use it if error channel is not specified. Should we use it instead?
There was a problem hiding this comment.
@mildred is the errChan ever used? It seems a bit redundant at this point
| @@ -226,14 +273,12 @@ func (dht *IpfsDHT) GetValues(ctx context.Context, key string, nvals int) ([]rou | |||
|
|
|||
| // run it! | |||
| _, err = query.Run(ctx, rtp) | |||
There was a problem hiding this comment.
whyrusleeping:
I think its probably bad that we ignore this error if we got at least one response back... I think this should be addressed separately
mildred:
Why do you say it's ignored? The fact is we must respond asynchronously to errors, and this one will appear at the very last moment. We cannot return it directly, this function is in a goroutine.
mildred/ipfs-objects@39c20b2#diff-50d2a8e6d11e785b103932ac6c926e72R274
There was a problem hiding this comment.
In the case where sentRes != 0, we never return the error from Run
There was a problem hiding this comment.
A few days ago, I understood more from your remark. I just fixed an issue where sentRes wasn't incremented when needed (it collects the number of results sent).
However, when there is no result, we might get an error from the lower layers, but that doesn't necessarily mean the GetValues must return an error. Instead it might mean that there is no result at all and we must not trigger an error. The previous version of the code silents some errors in this case, I see no reason to change that in this PR.
There was a problem hiding this comment.
@mildred right, i was mostly commenting that the existing behaviour might be incorrect. We can always change it in a different PR though.
|
Replaced by SearchValues. |
ghost
removed
the
For #47