Skip to content
This repository has been archived by the owner on Dec 7, 2019. It is now read-only.

make routing less DHT specific #21

Merged
merged 4 commits into from
Jun 2, 2018
Merged

make routing less DHT specific #21

merged 4 commits into from
Jun 2, 2018

Conversation

Stebalien
Copy link
Member

Motivation: According to the documentation/signature, it looks like GetValues
exists to return a bunch of different values from the ValueStore. However:

  1. In practice, we use it to tell the DHT how many peers it should contact (not
    because we actually want multiple values).
  2. It exposes non-valuestore related information (the peers from which we got
    the values).
  3. It doesn't correct peers that have the wrong values (it doesn't validate
    anything).
  4. Callers end up duplicating a lot of the GetValue code to select the best value.

This interface change removes GetValues and modifies GetValue to give us
everything we need. I've also left in a commented-out SearchValue method that
I'd like to eventually implement as a "progressive" GetValue (that keeps
returning better and better records).

@ghost ghost assigned Stebalien Apr 26, 2018
@ghost ghost added the in progress label Apr 26, 2018
Stebalien added a commit to libp2p/go-libp2p-kad-dht that referenced this pull request Apr 26, 2018
@Stebalien
Copy link
Member Author

I originally started working on this to try to make using pubsub as a ValueStore reasonable. Unfortunately, I couldn't quite get all of the abstractions in line. However, IMO, this change is still a step in the right direction.

Stebalien added a commit to libp2p/go-libp2p-kad-dht that referenced this pull request Apr 26, 2018
@vyzo
Copy link

vyzo commented Apr 27, 2018

It exposes non-valuestore related information (the peers from which we got the values).

Agreed on the sentiment that this leaks as an abstraction, but on the same time it can also be useful for users of the DHT.
Is there a substitute?

@Stebalien
Copy link
Member Author

Which information, the peers from which we got the values? At the moment, you can use the routing notification system.

@vyzo
Copy link

vyzo commented Apr 30, 2018

So coming back to the premise, are we sure we won't have any use case that is interested in multiple different values?

1. Handle key extraction.
2. Take a peer ID instead of a raw hash.
3. Comment that we should consider removing this.
@Stebalien Stebalien changed the title WIP: make routing less DHT specific make routing less DHT specific May 9, 2018
@Stebalien
Copy link
Member Author

So coming back to the premise, are we sure we won't have any use case that is interested in multiple different values?

There probably are but the semantics will likely be a bit different:

  1. The selector may look different (currently, we can always pick a single best value).
  2. We'll probably want to return at most k values instead of requiring k values.
  3. We'll probably want to use a channel to return them as we find them.

Basically, we may need a method with this name however, I'd hold off on that until we actually know what the semantics of that method will be.


// Expired is an option that tells the routing system to return expired records
// when no newer records are known.
var Expired Option = func(opts *Options) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

any particular reason this isnt just:

func Expired(opts *Options) error {
// ....
}

@whyrusleeping
Copy link
Contributor

This feels a little weird, and I do like the idea of being able to get multiple values out, but otherwise, this is probably a good step forward

@Stebalien Stebalien merged commit c131f85 into master Jun 2, 2018
@ghost ghost removed the in progress label Jun 2, 2018
@Stebalien
Copy link
Member Author

This feels a little weird, and I do like the idea of being able to get multiple values out, but otherwise, this is probably a good step forward

That's the idea behind the commented out search method. Basically, we'll want something more powerful than "give me K (possibly identical) values from N peers".

@Stebalien Stebalien deleted the feat/getvalue branch June 2, 2018 07:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants