Implement SearchValue by magik6k · Pull Request #174 · libp2p/go-libp2p-kad-dht

magik6k · 2018-07-17T21:51:49Z

This exposes new function which streams entries as they are found - https://github.com/libp2p/go-libp2p-routing/blob/master/routing.go#L61

Part of ipfs/kubo#5232
Replaces #49

TODO:

More tests
Error handling feels weird

Stebalien · 2018-07-17T23:55:06Z

Tell me when you want this reviewed.

magik6k · 2018-07-18T13:39:23Z

Should be ready

Stebalien · 2018-07-18T14:22:29Z

-		return nil, routing.ErrNotFound
+	responsesNeeded := 0
+	if !cfg.Offline {
+		responsesNeeded = getQuorum(&cfg)


Not sure if we should be applying the quorum in here. That's more of a GetValue thing. Really, we should probably have a separate "limit" option that defaults to -1. We can then get the quorum in GetValue and then apply it as the limit. Otherwise, we'll always stop after the default quorum (if not specified).

Note: by default, we should continue searching until we run out of peers.

Alternatively, we may just want to make GetValue and SearchValue orthogonal (using the same GetValues under the covers).

Stebalien · 2018-07-18T14:22:47Z

+		select {
+		case r, ok := <-responses:
+			if !ok {
+				responses = nil


I'd just use a break label.

It would lead to occasionally ignoring errors

Ah. I didn't see the && below.

Actually, do we even need an error channel? I'd just return an error up front (valid key, non-empty routing table, etc.) and then close when done. At that point, I don't see how we can really "fail". If the user wants more context, they can register a notifier.

Is that possible? It'll be a lot nicer to the user.

Stebalien · 2018-07-18T14:27:08Z


 // GetValues gets nvals values corresponding to the given key.
-func (dht *IpfsDHT) GetValues(ctx context.Context, key string, nvals int) (_ []RecvdVal, err error) {
+func (dht *IpfsDHT) GetValues(ctx context.Context, key string, nvals int) <-chan RecvdVal {


We may not want to change the API here. For now, I'd just add a private getValues function and make GetValue a simple wrapper.

magik6k · 2018-07-18T17:45:37Z

On quorum on SearchValue - I think it makes more sense than a limit option. As implemented the option does pretty much exactly what it did before, which is basically 'ask that many peers before giving a final value'. A limit option would imply that we expect multiple versions of a key, which usually/optimally shouldn't be the case.

Stebalien · 2018-07-18T18:03:29Z

On quorum on SearchValue - I think it makes more sense than a limit option. As implemented the option does pretty much exactly what it did before, which is basically 'ask that many peers before giving a final value'. A limit option would imply that we expect multiple versions of a key, which usually/optimally shouldn't be the case.

Yeah, you're probably right. However, I'd default to "continue till we run out of peers" instead of the default quorum of 16.

magik6k · 2018-07-18T18:46:01Z

Yeah, you're probably right. However, I'd default to "continue till we run out of peers" instead of the default quorum of 16.

Done

Stebalien · 2018-07-18T19:11:21Z

 		}

-		if err != io.EOF {
+		if err != routing.ErrNotFound {


this should be a context.ErrDeadlineExceeded. We'll probably need to check if the context was canceled in GetValue when the channel is closed.

Stebalien · 2018-07-18T20:14:39Z

+		if responsesNeeded < 0 {
+			responsesNeeded = 0
+		}
+		vals := make([]RecvdVal, 0, responsesNeeded)


This could grow very large (with no quorum). We may want to fire off a round of corrections ocationally (i.e., see 30 values, correct peers and reset). However, we can probably leave that as a TODO for now.

Alternatively, we could just cap responsesNeeded for now. Actually, that's probably the best thing to do. Cap it at 50 or something.

Leaving it unbounded could also cause a bunch of goroutines below.

Sign in to view

+		best = r
 	}
-	if len(recs) == 0 {
+


Stebalien · 2018-07-21T00:24:05Z

+					if err != nil {
+						continue //TODO: Do we want to do something with the error here?
+					}
+					if i != best && !bytes.Equal(v.Val, vals[best].Val) {


I will be either 0 or 1, not best.

Stebalien · 2018-07-21T00:24:43Z

+				if best > -1 {
+					i, err := dht.Validator.Select(key, [][]byte{vals[best].Val, v.Val})
+					if err != nil {
+						continue //TODO: Do we want to do something with the error here?


Log an error. This should never happen. Honestly, we could just panic (but I'd rather not).

Stebalien · 2018-07-21T00:25:51Z

+		out = append(out, val)
+	}
+
+	return out, nil


Needs to return this plus ctx.Error() (the context could have been canceled, etc.).

Stebalien · 2018-08-06T20:37:15Z

-	record "github.com/libp2p/go-libp2p-record"
-	routing "github.com/libp2p/go-libp2p-routing"
-	mocknet "github.com/libp2p/go-libp2p/p2p/net/mock"
+	"github.com/libp2p/go-libp2p-record"


Given that we never really use consistent naming, in package names, I'd rather leave the explicit names in-place.

Stebalien · 2018-08-06T20:37:50Z

 		}

-		if err != io.EOF {
+		if err != context.DeadlineExceeded {


Stebalien · 2018-08-06T21:12:26Z

+				if len(vals) < maxVals {
+					vals = append(vals, v)
+				} else {
+					i = (best + 1) % maxVals


i could still be out of bounds. It may be better to simply put the "best" value in a separate variable and then keep an array of peers to correct.

Stebalien · 2018-08-06T21:20:06Z

+}
+
+func (dht *IpfsDHT) getValues(ctx context.Context, key string, nvals int) (<-chan RecvdVal, error) {
 	eip := log.EventBegin(ctx, "GetValues")


This should probably go into GetValues. That may also help simplify/get rid of the done dance.

Stebalien · 2018-08-06T21:26:58Z

 			}
 			valslock.Lock()
-			vals = append(vals, rv)
+			vals <- rv


This should select on the context.

Stebalien

Missed two blocking channel writes.

Stebalien · 2018-08-07T18:19:59Z

+					}
+					if sel == 1 && !bytes.Equal(v.Val, best.Val) {
+						best = &v
+						out <- v.Val


Needs to select on the context.

Stebalien · 2018-08-07T18:20:04Z

+					if err := dht.Validator.Validate(key, v.Val); err == nil {
+						best = &v
+						out <- v.Val
+					}


Stebalien

LGTM. I have a few nits and cleanups but this should work. We can do that in a separate PR.

ghost assigned magik6k Jul 17, 2018

ghost added the status/in-progress In progress label Jul 17, 2018

magik6k force-pushed the feat/searchvalue branch 3 times, most recently from bcb8474 to 75bae8d Compare July 18, 2018 12:47

magik6k requested a review from Stebalien July 18, 2018 13:39

Stebalien reviewed Jul 18, 2018

View reviewed changes

magik6k force-pushed the feat/searchvalue branch from 4ca5152 to 09f5251 Compare July 18, 2018 17:34

magik6k changed the title ~~[WIP] Implement SearchValue~~ Implement SearchValue Jul 18, 2018

magik6k force-pushed the feat/searchvalue branch from 790b5ca to f02c32e Compare July 18, 2018 18:26

Stebalien reviewed Jul 21, 2018

View reviewed changes

magik6k force-pushed the feat/searchvalue branch from aeecd87 to c22e1e1 Compare August 3, 2018 21:00

magik6k requested a review from Stebalien August 3, 2018 21:03

Stebalien requested changes Aug 6, 2018

View reviewed changes

Stebalien requested changes Aug 7, 2018

View reviewed changes

magik6k added 10 commits August 10, 2018 12:06

Implement SearchValue

e260fe5

SearchValue: simplify error handling

64d1622

SearchValue: add a test

ad37d99

SearchValue: simplify error handling further

547c0a7

SearchValue: add backward-compatible GetValues

e28b133

SearchValue: Make -1 quorum mean infinite on get

307660c

SearchValue: make default quorum infinite for search

51c475d

SearchValue: address review

d72432c

SearchValues: more review addressing

6ca5dd7

SearchValue: respect ctx on output

246579e

magik6k force-pushed the feat/searchvalue branch from 3091c6b to 246579e Compare August 10, 2018 10:13

Stebalien approved these changes Aug 10, 2018

View reviewed changes

go fmt

cd3cac0

ghost assigned Stebalien Aug 10, 2018

Stebalien merged commit f6a03cb into master Aug 10, 2018

ghost removed the status/in-progress In progress label Aug 10, 2018

Stebalien deleted the feat/searchvalue branch August 10, 2018 20:55

magik6k mentioned this pull request Aug 30, 2018

SearchValue libp2p/go-libp2p-routing#25

Merged

4 tasks

vasco-santos mentioned this pull request Dec 9, 2018

2019 Q1 OKRs Planning libp2p/js-libp2p#293

Merged

Conversation

magik6k commented Jul 17, 2018

Uh oh!

Stebalien commented Jul 17, 2018

Uh oh!

magik6k commented Jul 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

magik6k commented Jul 18, 2018

Uh oh!

Stebalien commented Jul 18, 2018

Uh oh!

magik6k commented Jul 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Stebalien left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Stebalien left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants