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

docs: public rendezvous procedure #1203

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

docs: public rendezvous procedure #1203

wants to merge 9 commits into from

Conversation

lchenut
Copy link
Collaborator

@lchenut lchenut commented Sep 27, 2024

Three changes on this PR:

  • Remove old comments from old implementation.
  • Change advertise from method to proc.
  • add documentation to the public rendezvous procedures.

Related to these comments: #1183 (comment), #1183 (comment) and #1183 (comment)

await rdv.advertise(ns, ttl, rdv.peers)

proc requestLocally*(rdv: RendezVous, ns: string): seq[PeerRecord] =
## This procedure returns all the peers already registered on the
## given namespace. This function is synchronous
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is synchronous

Not sure this is necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@@ -555,6 +560,10 @@ proc requestLocally*(rdv: RendezVous, ns: string): seq[PeerRecord] =
proc request*(
rdv: RendezVous, ns: string, l: int = DiscoverLimit.int, peers: seq[PeerId]
): Future[seq[PeerRecord]] {.async.} =
## This async procedure request discovers and returns peers for
Copy link
Contributor

Choose a reason for hiding this comment

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

request discovers

Is it a typo?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@@ -555,6 +560,10 @@ proc requestLocally*(rdv: RendezVous, ns: string): seq[PeerRecord] =
proc request*(
rdv: RendezVous, ns: string, l: int = DiscoverLimit.int, peers: seq[PeerId]
Copy link
Contributor

Choose a reason for hiding this comment

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

it's better to use a more descriptive name for the parameter than l.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@@ -646,6 +655,10 @@ proc unsubscribeLocally*(rdv: RendezVous, ns: string) =
return

proc unsubscribe*(rdv: RendezVous, ns: string, peerIds: seq[PeerId]) {.async.} =
## The async unsubscribe procedure removes peers from a namespace by
## sending an "Unregister" message to each connected peer. The operation
## is bounded by a timeout for unsubscribing from all peers.
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this timeout?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants