feat(share/discovery): add Peers method #1451
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #1451 +/- ##
==========================================
- Coverage 56.02% 55.47% -0.55%
==========================================
Files 188 197 +9
Lines 11551 11997 +446
==========================================
+ Hits 6471 6655 +184
- Misses 4443 4683 +240
- Partials 637 659 +22
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
vgonkivs
force-pushed
the
extend_discovery
branch
from
c87096f
to
d59542b
Compare
Wondertan
previously approved these changes
Dec 8, 2022
There was a problem hiding this comment.
Note for everyone. This design has one problem, it does not allow us to get newly discovered peers in real-time, so protocols relying on it won't get them without retries. We are going this way for simplicity first and can improve with further iterations of the protocol.
vgonkivs
force-pushed
the
extend_discovery
branch
from
6c7c120
to
f80c7e0
Compare
Wondertan
previously approved these changes
Dec 8, 2022
distractedm1nd
previously approved these changes
Dec 8, 2022
Co-authored-by: Ryan <ryan@celestia.org>
vgonkivs
dismissed stale reviews from distractedm1nd and Wondertan
via
ad713bf
Wondertan
previously approved these changes
Dec 8, 2022
renaynay
reviewed
Dec 9, 2022
renaynay
approved these changes
Dec 9, 2022
distractedm1nd
approved these changes
Dec 9, 2022
Wondertan
approved these changes
Dec 9, 2022
walldiss
reviewed
Dec 9, 2022
| @@ -61,12 +71,24 @@ func (ps *limitedSet) Remove(id peer.ID) { | |||
| } | |||
| } | |||
|
|
|||
| func (ps *limitedSet) Peers() []peer.ID { | |||
| // Peers returns all discovered peers from the set. | |||
There was a problem hiding this comment.
I think it is worth mention in comment that Peer() will block if not peers are available until either ctx is Done or new peer discovered
Comment on lines
+56
to
+59
| select { | ||
| case ps.waitPeer <- p: | ||
| default: | ||
| } |
There was a problem hiding this comment.
Looks like if there are multiple waiters we will only notify one. I suggest for loop with return in default case of select.
Nit: Could refactor it to return error earlier and then.
5 tasks
vgonkivs
added a commit
that referenced
this pull request
Dec 9, 2022
<!-- Please read and fill out this form before submitting your PR. Please make sure you have reviewed our contributors guide before submitting your first PR. --> ## Overview Improvement for #1451 Notify multiple readers instead of one. ## Checklist <!-- Please complete the checklist to ensure that the PR is ready to be reviewed. IMPORTANT: PRs should be left in Draft until the below checklist is completed. --> - [x] New and updated code has appropriate documentation - [x] New and updated code has new and/or updated testing - [x] Required CI checks are passing - [x] Visual proof for any user facing features like CLI or documentation updates - [x] Linked issues closed with keywords
renaynay
pushed a commit
to renaynay/celestia-node
that referenced
this pull request
Dec 9, 2022
## Overview Resolves celestiaorg#1449 ## Checklist - [x] New and updated code has appropriate documentation - [x] New and updated code has new and/or updated testing - [x] Required CI checks are passing - [x] Visual proof for any user facing features like CLI or documentation updates - [x] Linked issues closed with keywords Co-authored-by: Ryan <ryan@celestia.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Resolves #1449
Checklist