Skip to content

Peer Discovery Interface#123

Merged
alexh merged 4 commits intomasterfrom
peer-disc
Feb 24, 2019
Merged

Peer Discovery Interface#123
alexh merged 4 commits intomasterfrom
peer-disc

Conversation

@alexh
Copy link
Contributor

@alexh alexh commented Feb 10, 2019

I have added a peer discovery interface modeled after the interface used in the go repo

@codecov-io
Copy link

Codecov Report

Merging #123 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #123   +/-   ##
=======================================
  Coverage   89.78%   89.78%           
=======================================
  Files          33       33           
  Lines         685      685           
=======================================
  Hits          615      615           
  Misses         70       70

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5af7079...d47c423. Read the comment docs.

@zaibon
Copy link
Contributor

zaibon commented Feb 11, 2019

I actually have some code layout around that implement these interface as well as some basic testing around kadmelia package. I'll clean this up and create a PR

"""
Find peers on the networking providing a particular service
:param service: service that peers must provide
:return: peerstore containing found peers on the network
Copy link
Contributor

Choose a reason for hiding this comment

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

If we look at the return value of the go implementation: https://github.com/libp2p/go-libp2p-discovery/blob/master/interface.go#L19
find_peers could return a generator or PeerInfo instead of a PeerStore
That would prevent to load too much in memory in case there are a lot of peers to be discovered

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in new commit

from abc import ABC, abstractmethod
# pylint: disable=too-few-public-methods

class IDiscoverer(ABC):
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't it be : IAdvertiser ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@alexh
Copy link
Contributor Author

alexh commented Feb 24, 2019

I actually have some code layout around that implement these interface as well as some basic testing around kadmelia package. I'll clean this up and create a PR

@zaibon We would welcome a PR on this, but in the meantime our team will be focusing on pubsub until the discv5 spec is more solidified, then we will implement discv5.

@alexh alexh requested a review from zixuanzh February 24, 2019 23:33
Copy link
Contributor

@zixuanzh zixuanzh left a comment

Choose a reason for hiding this comment

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

LGTM, we will close this PR for now. @zaibon we look forward to your contribution!

@alexh alexh merged commit 17c778d into master Feb 24, 2019
This was referenced Feb 25, 2019
@pacrob pacrob deleted the peer-disc branch November 21, 2024 19:02
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.

4 participants