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

Feat/add discovery client #3

Merged
merged 5 commits into from
Jul 8, 2019

Conversation

aschmahmann
Copy link

Added a stateful client that implements the libp2p discovery interface.

@vyzo vyzo self-requested a review May 31, 2019 17:47
Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

It's a great start.
The FindPeers method will need to be modified however so that it stores the cookie and iterates, otherwise it will always get just the first set of peers.

discovery_client.go Outdated Show resolved Hide resolved
discovery_client.go Outdated Show resolved Hide resolved
discovery_client.go Outdated Show resolved Hide resolved
discovery_client.go Outdated Show resolved Hide resolved
…egistered records.

Default TTL for discovery client increased
discovery client now utilizes server cookie for added efficiency
Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

This still needs work...
Also, can you call the discovery module file just discovery? discovery_client is a mouthful.
And try to shorten variable/type names as well.

discovery_client.go Outdated Show resolved Hide resolved
discovery_client.go Outdated Show resolved Hide resolved
discovery_client.go Outdated Show resolved Hide resolved
discovery_client.go Outdated Show resolved Hide resolved
discovery_client.go Outdated Show resolved Hide resolved
discovery_client.go Outdated Show resolved Hide resolved
discovery_client.go Outdated Show resolved Hide resolved
discovery_client.go Outdated Show resolved Hide resolved
@aschmahmann
Copy link
Author

aschmahmann commented Jun 4, 2019

This still needs work...
Also, can you call the discovery module file just discovery? discovery_client is a mouthful.
And try to shorten variable/type names as well.

No problem, I've found that longer names are more self descriptive (and not out of style in comparison to a number of Golang packages) and that with autocomplete they aren't actually a pain. However, I also have no interest in debating variable names as long as they are descriptive enough to get the point across so that interested reviewers can propose new names for them it's a simple approve from me.

Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

this is looking good.

discovery.go Outdated Show resolved Hide resolved
discovery.go Outdated
}

type discoveredPeerCache struct {
cachedRecs map[peer.ID]*record
Copy link
Contributor

Choose a reason for hiding this comment

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

and this just recs, it's a cache and these are named records.

discovery.go Outdated Show resolved Hide resolved
@aschmahmann
Copy link
Author

@vyzo switched sync.Map with Map + RWMutex like we did in pubsub

@vyzo vyzo merged commit 7371441 into libp2p:implement-spec Jul 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants